tools: add -F flag for fixing lint issues

[Trott]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

tools

Description of change

Add -F flag to jslint.js which enables the automatic fixing of
issues that are fixable.

R=@mscdex

[mscdex]

Does this line just remove the fixed lint errors from the report?

[Trott]

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.

[mscdex]

Ok, I was just surprised it has enough information in the report to fix errors without needing the actual cli instance.

[mscdex]

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 ...

[Trott]

@mscdex wrote:

hrmm... that's odd, it took almost a minute to lint that time ...

That's inline with other linter runs.

Job 2232 (this one): 51 seconds
Job 2231: 65 seconds
Job 2230: 47 seconds
Job 2229: 48 seconds
Job 2228: 47 seconds
Job 2227: 48 seconds
Job 2226: 47 seconds
Job 2225: 50 seconds
Job 2224: 48 seconds
Job 2223: 47 seconds

The time includes doing the git checkout and probably other things.

[mscdex]

I thought we were consistently getting ~31 seconds after the switch to tools/jslint.js. I guess something else changed on that system in the meantime. Oh well.

[jasnell]

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

[mscdex]

@jasnell I agree it would be nice if eslint could instead mark the problems that were fixed and the report formatters could show this.

[silverwind]

they may forget to add it and commit it to pick up those changes

Not really an issue with git commit -a.

Should we expose fixing through something like make jslint-fix?

[Trott]

@silverwind asked:

Should we expose fixing through something like make jslint-fix?

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. :-)

[jasnell]

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 ;-)

[Trott]

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 -F and it reports no errors, run it again without -F to make sure any fixes didn't create new errors. I've only seen this happen with indentation problems. Other than that, as far as I know, it's a reliable time-saver.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/2454/

[silverwind]

process.argv.includes('-F')?

[silverwind]

Or maybe put the result into above object, like fix: process.argv.includes('-F'), reducing the changes around here to a single line.

[Trott]

@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.

[silverwind]

Oh, right. Keep it as-is for LTS then.

[Trott]

Landed in db3db07