tools: add -F flag for fixing lint issues #6483
Conversation
Add `-F` flag to `jslint.js` which enables the automatic fixing of issues that are fixable.
|
|
||
| // If we were asked to fix the fixable issues, do so. | ||
| if (cliOptions.fix) | ||
| CLIEngine.outputFixes(report); |
There was a problem hiding this comment.
Does this line just remove the fixed lint errors from the report?
There was a problem hiding this comment.
No, that appears to be done by passing fix: true to the constructor. Much to my surprise, passing that to the constructor is not enough to actually fix the fixable issues. That's what this line does.
There was a problem hiding this comment.
Ok, I was just surprised it has enough information in the report to fix errors without needing the actual cli instance.
|
LGTM if linter is ok with it: https://ci.nodejs.org/job/node-test-linter/2232/ EDIT: hrmm... that's odd, it took almost a minute to lint that time ... |
|
@mscdex wrote:
That's inline with other linter runs. Job 2232 (this one): 51 seconds The time includes doing the git checkout and probably other things. |
|
I thought we were consistently getting ~31 seconds after the switch to |
|
Nice. Seems to work well but the lack of feedback that things were fixed in specific files could lead to issues... for instance, if someone doesn't know that a certain file was fixed by the linter, they may forget to add it and commit it to pick up those changes... or, the linter may have done something wrong and the person may need to undo those changes for whatever reason. In any case +1 to this. Appears to work as expected. LGTM |
|
@jasnell I agree it would be nice if eslint could instead mark the problems that were fixed and the report formatters could show this. |
Not really an issue with Should we expose fixing through something like |
|
@silverwind asked:
Eventually, that could be a good idea. For now, I think I'd like to dog-food it for a while. There are definite pitfalls, especially around indentation, and I'd rather see how that does or doesn't affect things for a while. Basically, if you're in the loop enough to be aware of the flag, then maybe you can help dog-food it too. :-) |
given the number of times I have to catch myself fixing the same lints I'll definitely give it a whirl ;-) |
My one tip is this: If you run it with |
| }; | ||
|
|
||
| // Check if we should fix errors that are fixable | ||
| if (process.argv.indexOf('-F') !== -1) |
There was a problem hiding this comment.
process.argv.includes('-F')?
There was a problem hiding this comment.
Or maybe put the result into above object, like fix: process.argv.includes('-F'), reducing the changes around here to a single line.
There was a problem hiding this comment.
@silverwind wrote:
process.argv.includes('-F')?
I don't feel strongly about it, but I went with indexOf() !== -1 over includes() so that this could be used in the LTS branch and to be consistent with line 47 and elsewhere in the file that was already using indexOf() !== -1 for existence.
There was a problem hiding this comment.
Oh, right. Keep it as-is for LTS then.
Add `-F` flag to `jslint.js` which enables the automatic fixing of issues that are fixable. PR-URL: #6483 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
|
Landed in db3db07 |
Add `-F` flag to `jslint.js` which enables the automatic fixing of issues that are fixable. PR-URL: #6483 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Add `-F` flag to `jslint.js` which enables the automatic fixing of issues that are fixable. PR-URL: nodejs#6483 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Checklist
Affected core subsystem(s)
tools
Description of change
Add
-Fflag tojslint.jswhich enables the automatic fixing ofissues that are fixable.
R=@mscdex