Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Determine fix results based on exit code #777

Merged
merged 3 commits into from
Jan 9, 2017

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Jan 5, 2017

Fixes #511

Description

This adjusts the response of running Linter eslint: fix or fixing automatically on save, so that unexpected runtime errors are correctly printed in a notification.

This PR also no longer treats an incomplete fix as requiring a warning. A success message is given whether the fix is total or partial, with only the wording being altered to reflect the result. As I mention in the commit message details, I believe eslint.execute used to throw when autofix did not fix all of the errors in a file. It no longer does, so we need to rely on the exit code instead.


This change is Reviewable

@IanVS IanVS requested a review from Arcanemagus January 5, 2017 04:06
@IanVS IanVS force-pushed the issue511-fix-error-notification branch from 577b5d7 to 913d0e1 Compare January 5, 2017 04:10
@IanVS
Copy link
Member Author

IanVS commented Jan 5, 2017

Hm, I just noticed that this change will cause linter-eslint to display a warning of: No ESLint configuration found. if Fix errors on save is checked or a manual fix is run, even if Disable when no ESLint config is found is checked.

It's nice to get the warning in the manual case, but I think saving a file should not show that message if the Disable option is checked. I'll need to work on that.

@IanVS
Copy link
Member Author

IanVS commented Jan 5, 2017

OK, the latest commit should help. We will no longer attempt to perform a fix job when disableWhenNoEslintConfig is true and there's no config file found. But, we will still attempt it if a manual Linter Eslint: fix command is run, and a helpful message of No ESLint configuration found. will be shown.

}
return 'Linter-ESLint: Fix attempt complete, but linting errors remain.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"linting issues" maybe? The remaining messages aren't necessarily errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, to get an error code other than 0, means there are errors. If there are only warnings, eslint will exit with a 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't know that. errors works just fine then!

// Do not try to fix if linting should be disabled
const fileDir = Path.dirname(filePath)
const configPath = getConfigPath(fileDir)
if (configPath === null && atom.config.get('linter-eslint.disableWhenNoEslintConfig')) return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting should be observed instead of retrieved every time.

IanVS added 3 commits January 7, 2017 21:39
Perhaps in the past ESLint threw an error if `--fix` did not fix all of
the errors in a file.  Or, maybe this logic was flawed from the start.
Either way, this change just looks at the exit code (`0` if there are 
no linting errors left, `1` if there are), and returns an appropriate
string.  Both cases are really success messages, there were no errors.

If there was an error for some reason, we need to print the `err.message`
string, because `addWarning` expects a string, not an error object.
If `disableWhenNoEslintConfig` is disabling linting, we should also
not be trying to perform an autofix.
@IanVS IanVS force-pushed the issue511-fix-error-notification branch from a7e073d to d7ef6d4 Compare January 8, 2017 02:39
@IanVS
Copy link
Member Author

IanVS commented Jan 8, 2017

OK, @Arcanemagus, I made the fix you asked for, and rebased on master. I think this is ready-to-go.

Copy link
Member

@Arcanemagus Arcanemagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merge at will @IanVS 😉

@IanVS IanVS merged commit 321dc54 into master Jan 9, 2017
@Arcanemagus Arcanemagus deleted the issue511-fix-error-notification branch January 30, 2017 19:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants