Use ESLint's CLIEngine API rather than the command line (fixes #797) #873

not-an-aardvark · 2017-04-04T05:46:53Z

This updates linter-eslint to use ESLint's CLIEngine API rather than the private cli object. This makes the logic for interacting with ESLint much simpler.

I'm not very familiar with the linter-eslint codebase or Atom plugins in general, so be sure to review this thoroughly.

…nter#797)

Arcanemagus

Other than the rather funny decisions git made on the directories it was making the cli.js -> app.js moves to/from, this looks great to me.

IanVS

I have a few comments and questions, but overall this looks pretty solid. There's only one thing I think we have to address for sure (the check if fix was successful).

I'm actually somewhat surprised by how few changes you had to make here. For some reason I assumed it would be a bigger diff. Nice going.

IanVS · 2017-04-06T03:25:49Z

src/worker.js

  } else if (type === 'fix') {
-    job.response = fixJob(argv, eslint)
+    job.response = fixJob({ cliEngineOptions, contents, eslint, filePath })


Question: Is the reason you're passing a currently-always-undefined contents into fixJob because you expect that in the future we would pass unsaved text into the fix job? For some reason I thought that ESLint wouldn't allow --fix for stdin, but maybe it does when using executeOnText? Could you talk a little about your thoughts here on passing contents to fixJob?

Actually, it's because I didn't realize contents was always undefined. 😄

It's true that ESLint doesn't allow --fix from stdin on the command line, but this isn't a problem when calling executeOnText because it accepts a filename argument. So using executeOnText followed by CLIEngine.outputFixes would allow us to pass unsaved text (if we wanted to do that).

Notably, executeOnText also accepts a warnIgnored argument, which can be used to disable "file ignored because of matching ignore pattern" messages. If we switch to using contents and used executeOnText instead of executeOnFiles, we could also resolve the issue that you brought up in #873 (comment) without needing to bring back the message list.

(I suppose another way to do this would be to read the file from the filesystem ourselves and call executeOnText on it to get rid of the ignore warnings.)

I think we should explore using executeOnText for fixes, but in a separate PR. I'd like to keep this PR small in scope so we're not changing too much at once.

Ok, in that case should I add back the list of ignore messages for now?

That would be my preference, yes.

IanVS · 2017-04-06T03:43:43Z

src/worker.js

+
+  eslint.CLIEngine.outputFixes(report)
+
+  if (!report.results.length || !report.results[0].messages.length) {


Unfortunately, this won't work if a user tries to fix an ignored file. We will get back a warning from ESLint that the file is ignored, and therefore the message displayed will be:

Linter-ESLint: Fix attempt complete, but linting errors remain.

which is less than ideal. I really wanted to be happy about deleting ignoredMessages, but I think we still need it, at least for this situation. We could even be smart and show a meaningful message if we determine that the file the user tried to fix is being ignored.

Perhaps we can move to executeOnText for fix jobs at some point and turn off warnIgnored, as you mentioned in #873 (comment), but for now let's keep the ignoredMessages and use them here.

If I'm understanding this correctly in that executeOnText works for this, couldn't we just send in contents from the fix jobs? We already have a reference to the editor in both places one is requested so this should be pretty simple.

Nevermind, just read the comments below, moving that to a separate PR works for me.

Yes, that's the proposal. I also think it would be easy, but I'd like it to be done in a separate PR to isolate the changes.

IanVS · 2017-04-06T03:45:13Z

src/worker.js

-    job.response = lintJob(argv, contents, eslint, configPath, config)
+  const noProjectConfig = (configPath === null || isConfigAtHomeRoot(configPath))
+  if (noProjectConfig && config.disableWhenNoEslintConfig) {
+    job.response = []


Good call moving this check down here, it should definitely apply to lint jobs and fix jobs. 👍

IanVS · 2017-04-06T03:47:12Z

src/worker.js

@@ -54,16 +39,19 @@ create().onRequest('job', ({ contents, type, config, filePath, projectPath, rule
  const configPath = Helpers.getConfigPath(fileDir)
  const relativeFilePath = Helpers.getRelativePath(fileDir, filePath, config)

-  const argv = Helpers.getArgv(type, config, rules, relativeFilePath, fileDir, configPath)
+  const cliEngineOptions =
+    Helpers.getCLIEngineOptions(type, config, rules, relativeFilePath, fileDir, configPath)


I'm not sure how @Arcanemagus feels about this one, but personally I would prefer avoiding the long-line error by formatting it as:

const cliEngineOptions = Helpers.getCLIEngineOptions( type, config, rules, relativeFilePath, fileDir, configPath )

That's how I would prefer it as well, but the styling in this project already has issues as far as I'm concerned so I just left it for a future update of the style 😛.

IanVS · 2017-04-06T03:49:17Z

src/worker.js

-  'File ignored by default. Use "--ignore-pattern \'!bower_components/*\'" to override.',
-]
+function lintJob({ cliEngineOptions, contents, eslint, filePath }) {
+  const cliEngine = new eslint.CLIEngine(cliEngineOptions)


I wonder, are we going to pay a performance penalty here by creating a new cliEngine each time we perform a linting job? If there's much overhead here, maybe it makes sense to memoize it, and only create a new cliEngine when the options change. What do you think?

That seems like it could be a good idea. It looks like CLIEngine constructor checks the --rulesdir directory for updates only once when it's constructed, so this would mean that any changes to the rules in the rules directory wouldn't be reflected immediately. (I suspect it's pretty rare for users to modify the rules directory in the common case.)

Also, it's worth noting that the cli API that we previously used constructs a new instance of CLIEngine on every invocation too, so I doubt this will hurt performance compared to the previous behavior (although there might be room for improvement).

Cool, let's leave this how it is and possibly try to memoize it in another PR.

IanVS · 2017-04-06T05:19:09Z

spec/linter-eslint-spec.js

+    it('will not give warnings when autofixing the file', () => {
+      waitsForPromise(() =>
+        atom.workspace.open(ignoredPath).then(editor =>
+          fix(editor).then(result => expect(result).toBe('Linter-ESLint: Fix complete.'))


This works for now, and it was the previous behavior, but I think a better thing to do would be to actually tell the user that nothing happened because the file was ignored. :)

It used to be that way, but users complained, which is why it's this way in the first place. Can't please everyone I guess.

Personally I would prefer to have the messages shown myself.

I think you mean we used to show the 'file ignored' messages when linting. And yeah, that's noisy. But when a user manually runs the fix command, we should show a meaningful message. I'll push a PR later if I can get around to it.

Arcanemagus · 2017-04-06T05:29:23Z

Approved and merging, thanks again! I can't believe this ended up being such a simple change.

IanVS · 2017-04-06T05:37:17Z

Agreed. Thanks @not-an-aardvark! We should have tried this a lot sooner. 😂

Use ESLint's CLIEngine API rather than the command line (fixes AtomLi…

154a63b

…nter#797)

Arcanemagus requested review from Arcanemagus and IanVS April 4, 2017 16:32

Arcanemagus added the enhancement label Apr 4, 2017

Arcanemagus approved these changes Apr 4, 2017

View reviewed changes

Arcanemagus mentioned this pull request Apr 5, 2017

Package activation time causes significant slowdown for new Atom windows #871

Closed

3 tasks

IanVS suggested changes Apr 6, 2017

View reviewed changes

not-an-aardvark added 2 commits April 6, 2017 00:40

Handle long line in a better way

8d366ca

Commit message ignored because of a matching ignore pattern

b2c9f76

IanVS reviewed Apr 6, 2017

View reviewed changes

IanVS approved these changes Apr 6, 2017

View reviewed changes

Arcanemagus merged commit b6bdeb1 into AtomLinter:master Apr 6, 2017

not-an-aardvark deleted the use-node-api branch April 6, 2017 05:32

IanVS mentioned this pull request May 1, 2017

Preserve .eslintcache on fixes #898

Merged

Arcanemagus mentioned this pull request May 2, 2017

Utilize warnIgnored #900

Closed

Arcanemagus mentioned this pull request Jun 14, 2017

Path case sensitivity issue causing require/import related plugin false errors #934

Closed

not-an-aardvark mentioned this pull request Jul 30, 2017

ESLint v5 Wish List eslint/eslint#8972

Closed

Arcanemagus mentioned this pull request Nov 8, 2017

Ability to set path to ESLint #1051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ESLint's CLIEngine API rather than the command line (fixes #797) #873

Use ESLint's CLIEngine API rather than the command line (fixes #797) #873

not-an-aardvark commented Apr 4, 2017

Arcanemagus left a comment

IanVS left a comment •

edited

Loading

IanVS Apr 6, 2017

not-an-aardvark Apr 6, 2017

IanVS Apr 6, 2017

not-an-aardvark Apr 6, 2017

IanVS Apr 6, 2017

IanVS Apr 6, 2017

IanVS Apr 6, 2017

Arcanemagus Apr 6, 2017

Arcanemagus Apr 6, 2017

IanVS Apr 6, 2017

IanVS Apr 6, 2017

IanVS Apr 6, 2017

Arcanemagus Apr 6, 2017

IanVS Apr 6, 2017

not-an-aardvark Apr 6, 2017

IanVS Apr 6, 2017

IanVS Apr 6, 2017

Arcanemagus Apr 6, 2017

IanVS Apr 6, 2017

Arcanemagus commented Apr 6, 2017

IanVS commented Apr 6, 2017


		eslint.CLIEngine.outputFixes(report)

		if (!report.results.length \|\| !report.results[0].messages.length) {

Use ESLint's CLIEngine API rather than the command line (fixes #797) #873

Use ESLint's CLIEngine API rather than the command line (fixes #797) #873

Conversation

not-an-aardvark commented Apr 4, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

IanVS left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus commented Apr 6, 2017

IanVS commented Apr 6, 2017

IanVS left a comment •

edited

Loading