-
-
Notifications
You must be signed in to change notification settings - Fork 73
Chore: Refactor the codebase (fixes #220) #261
Conversation
|
LGTM |
soda0289
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this looks great! I like how you broke util functions into its own files. We could probably start to do that with the node converts as well at some point.
Merging this in will cause all current PR to need rebase. Not sure what order will work best.
.travis.yml
Outdated
| node_js: | ||
| - "0.12" | ||
| - 4 | ||
| - 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add Node v7 as well?
lib/node-utils.js
Outdated
| /** | ||
| * Expose the enum of possible TSNode `kind`s. | ||
| */ | ||
| exports.SyntaxKind = SyntaxKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe we should move the exports to the bottom of the file and do it all in one object. That way it might be easier to know what a file exports. If this was typescript we could just export the function or variables but I don't care too much.
|
LGTM |
|
@soda0289 Yes fair point about the readability of exports, thanks! I have addressed that, and dialed up the linting strictness to fully match eslint/eslint (caught a lot of additional formatting points in test and Makefiles etc). I updated the travis to run on 4, 5, 6 and 7, again to match eslint/eslint. With regards to merging, I would love to get this in ASAP, and then continue working on more fixes. Are you happy to close out your old PRs and start branching off from this work? |
|
Yea I can do that. |
|
Thanks, sorry that this is awkward one - long overdue! I think I will get Pajn's breaking change one in first, then rebase this and merge it. Then we could do an actual release I think, because all the breaking changes will be in. |
|
Could we merge this into: I would like to get the comment scanner fixed before we release. Could you review that PR? |
|
LGTM |
|
Both of those are in now, thanks! |
|
Lets merge this then. I'll rebase the comment scanner code off of this change. |
|
🎉 |
This is major refactoring of the structure of the core codebase.
There are no changes to any existing test cases.
Summary of changes:
>=4to align witheslint/eslintobject-assigndependency, and replace its usage with nativeObject.assignfeatures.jsandtoken-translator.js- they were never usedeslint/eslintast-node-types.jsto include all known convertedESTreeNodetypeserrorOnUnknownASTTypeparser option to throw if a generatedESTreeNodetype is not found inast-node-types.js(this is disabled by default, but enabled in tests)node-utils.js, to make the codebase much easier to reason about, especially w.r.t. which helpers mutate the current node, and which are pure functionsts.*helper functions into abstractions withinnode-utils.jsso that it is clear what our dependencies on TypeScript actually arefindFirstMatchingChildandfindFirstMatchingAncestorwhich take inpredicatefunctions, and are built upon by other utility functions. These also allow me to remove somets.*dependencies from the codebaseconvert()function into its own file,convert.js, and increase what is explicitly passed into it rather than just closing over all outer scopes. Again, this greatly helps with clarity and focused responsibilityconvert.js, taking advantage ofletandconstfor variable declarations. No more battling to come up with unique variable names within a 1000+ line function!