feat/eslint #33
Merged
feat/eslint #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Refactored the Prettier runner and (and ESLint) to use the same function for check and apply, but only write the reformatted code if the |
|
Nice, looks great so far! I really like having the output from different tools show up in the same place. @varl since this is a draft PR are you looking for any specific feedback or assistance? Also, how's the performance? I could see this slowing down with large numbers of files since the different parsers are run in series. We could try to parallelize them with streams or separate processes if it's slow, but if it's currently fast that would be premature. |
Yeah, I think it's pretty nice getting a list of issues for each file. 💃
I decided to publish this as a draft to be transparent, and get some feedback on the way the feature is designed. It changes a bunch of internals and lays the foundation for additional tools that works on source code, so if something stands out as a Bad Idea™️ it would be nice to catch it early.
Performance is OK for general (staged files) use. Needs more testing in various situations to make sure. I would like to defer running in parallel for now (more on that below). That said, it is pretty slow when the amount of content to parse increases. When using Maintenance app (28k LoC, 352 files):Usage analytics (2.5k LoC, 54 files):Parallel executionRunning the The tricky part is running The flow I think might make sense would be:
Writing that down there's actually a problem with the implementation since it uses the same source as input to all the tools. I think it makes sense to first let ESLint run, then return the modified (if Programmatically embedding the order of tools centrally will also make it easier to use. |
Implemented this and a nice side-effect is that it only writes (and thus stage) files if there are no errors which cannot automatically be fixed. This leaves the tree intact in case of any errors which is nice. I'm going to clean it up the implementation a bit though. |
|
Added some performance stats when run with This output is from running |
|
@amcgee This is ready for review. |
|
When only the single file was actually changed. Also, maybe we should call |
|
@amcgee Ok try out the changes in 1527f8d. I've changed the response object from the tools to track if the source code has been modified or not, and only write and staged the files that were modified: |
|
ESLint strips out any violations that can be automatically applied from its report, so it is easier for us to align Prettier with this than the other way around. Or, we have to run ESLint twice: one with The reason I did not go this route is simply because it takes twice as long to run. |
Good idea, done in b99e55d |
|
@amcgee This should give correct results now. |
amcgee
approved these changes
Apr 24, 2019
|
LGTM!! |
dhis2-bot
pushed a commit
that referenced
this pull request
Apr 24, 2019
# [3.1.0](v3.0.1...v3.1.0) (2019-04-24) ### Features * add support for eslint for code style enforcement ([#33](#33)) ([85cf53a](85cf53a))
|
🎉 This PR is included in version 3.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am working on adding ESLint to the JS standards.
Principles
I am careful to only include only essential rules, as any rules added will be considered errors.
In the case the rules require manual intervention, they should be backed by a decision.
Auto fixable rule adoption can be less strict, but should be carefully considered nonetheless.
Design
To allow for multiple tools to scan the same source for warts I have created the
all-js.jsmodule, which executes all the tools (eslint.js,prettier.js, etc.) on the given files.The tools return an array of messages with violations for each file, which is then flattened before returning it, so all messages are grouped per file.
The
checkandapplycommand modules now useallJs.apply(files)andallJs.check(files)to execute their respective modes.Example output
Errors with messages
Apply all standards, with errors
Errors but quiet:
All Good
All Good & Quiet
Todo
all-jsMessageobject, perhaps a class to ensure consistencyWent with a different option, simplified the message object.
sourceTypefor ESLintUsing
babel-eslintseems to have fixed the problems with ESLint.