Make Linter Eslint: Fix File to use the same ESLint as when linting
#307
Conversation
| global: atom.config.get('linter-eslint.useGlobalEslint'), | ||
| nodePath: atom.config.get('linter-eslint.globalNodePath'), | ||
| } | ||
| const eslintDir = findEslintDir(params) |
There was a problem hiding this comment.
I would highly recommend that we do not do this in the main process, the sync file system operations and sync execution is gonna make the editor jam for the user
There was a problem hiding this comment.
OK, I'll have to study up on how to use a worker. Any tips?
|
Hm, not sure why CI is failing here. Please give this a thorough review. It seems to work when I test locally, but without specs it's hard to know for sure that I haven't borked anything up. |
|
CI is failing currently since |
| } | ||
|
|
||
| const modulesPath = find(params.fileDir, 'node_modules') | ||
| const eslintignoreDir = Path.dirname(find(params.fileDir, '.eslintignore')) | ||
| let eslintNewPath = null | ||
| if (modulesPath) { | ||
| process.env.NODE_PATH = modulesPath | ||
| } else process.env.NODE_PATH = '' | ||
| require('module').Module._initPaths() |
There was a problem hiding this comment.
Question
What does this line do?
There was a problem hiding this comment.
process.env.NODE_PATH change is not recognized by the node module loader, this line makes sure it does
|
@steelbrain & @Arcanemagus, sorry for the rather large PR here. This does several things:
Please let me know if you'd like to see more tests, or if this needs to be broken up. |
| } | ||
|
|
||
| if (this.worker === null) { | ||
| return; |
There was a problem hiding this comment.
No semi-colons please 😛
There was a problem hiding this comment.
It's a hard habit to break. :) We should turn on semi: never. In fact, I'll do that now and fix any errors. (Will be a good test of the --fix functionality too!)
There was a problem hiding this comment.
Personally I like having them in there, but can live without them. Just as long as the files are consistent 😛.
|
LGTM other than the null fix worker noted above. |
Fixes #300 This allows the `Linter Eslint: Fix file` command to use the same ESLint which is being used for linting the files, avoiding problems when plugins or shared configs are used. This also puts the execution of ESLint for error fixing onto the worker, which will free up the UI from being blocked. It also adds messages to indicate the success or partial success of the operation. ESLint will report a 0 exit code only if all linting errors are removed. Otherwise, if any linting errors remain, it will exit with a 1, in which case a warning will be displayed to indicate the operation is complete, but there are still errors. Also, this commit abstracts more methods into a helper, for reusability and testability.
Without this change, when an absolute path is provided, the leading slash is used as a separator and the first element of the chunks array is an empty string. With this change, the first element is replaced with a path separator, so that the reconstructed currentDirectory is an absolute path, as desired.
Instead of writing each subdirectory as a separate argument (which wasn’t being done correctly in the tests anyway), expect a single argument, `path`, which is a UNIX-style path.
| let prefixPath = null | ||
|
|
||
| function findEslintDir(params) { | ||
| const eslintPathLocal = Path.join(FS.realpathSync(Path.join(__dirname, '..')), 'node_modules', 'eslint') |
There was a problem hiding this comment.
Because the value of this doesn't change and syscalls are expensive, can we move this in upper scope please?
Bringing it out into the function’s closure will avoid repeated syscalls.
Make `Linter Eslint: Fix File` use the same ESLint as when linting
|
👏 |
Moves the execution of
eslint --fixto the worker, and uses the same eslint resolution as linting.Fixes #300