build: use generic names for linting tasks #15272

seishun · 2017-09-08T12:57:08Z

"jslint" is the name of a tool that actually is not used, which can cause confusion.

(I installed the jslint extension in VSCode and it took me a while to figure out why it's not working)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Trott

<deleted, I meant to request changes, not approve, sorry>

Trott

An explanation: jslint isn't supposed to be referring to the underlying tool and was never (as far as I know) intended that way. It's intend to be referring to the action. So jslint is correct. Running make jslint executes linting for js files.

It's unfortunate that jslint is readily identifiable as an existing tool.

I'm pretty sure this change has been proposed before and was rejected. However, (if I'm not misremembering all that) the fact that it has come up at least twice now suggests that the name is genuinely bothersome to some people, so maybe it is worth changing?

Except for the fact that now everyone has to know to run make eslint all of a sudden when they've been running make jslint forever. And tutorials are now wrong. And so on

I'm definitely -1 on getting rid of make jslint entirely. But if you wanted to add a make eslint alias that just runs make jslint, I wouldn't stop it. It does seem to be not very future-proof. One day we'll switch to another underlying tool and need to change the command again....

seishun · 2017-09-09T09:33:15Z

@Trott I'd like to eventually get rid of "jslint" in the codebase though. Would you be less opposed if make jslint displayed an error telling the user to use make eslint instead?

Having make eslint and make jslint be aliases of each other could be confusing. Someone might erroneously think that the latter runs JSLint, while the former runs ESLint.

Trott · 2017-09-09T19:17:42Z

@seishun How about we leave the underlying tool out of the naming entirely? Maybe something like this?:

Given that:

make test tests (nearly) everything.
make test-addons and similar variants only test addons, etc.
make lint lints everything.

Perhaps instead of make jslint and make cpplint, we should reformat like the test-* stuff?:

make lint-js will do what make jslint currently does.
make lint-cpp will do what make cpplint currently does.

We can have a period of time where jslint is an undocumented alias for lint-js. After a period of time, jslint emits a warning pointing at the new name but is still an effective alias. At some point after that, it emits the warning and does nothing else. And maybe it stays that way forever or maybe we remove the alias at some point after that.

seishun · 2017-09-10T13:22:58Z

@Trott I'm open to that, although I'm not totally comfortable with inventing a generic term for linting JS. Let's see what other @nodejs/collaborators think.

We can have a period of time where jslint is an undocumented alias for lint-js. After a period of time, jslint emits a warning pointing at the new name but is still an effective alias.

What harm is there in introducing a warning right away?

mscdex · 2017-09-10T13:32:21Z

I think keeping 'jslint' is best, especially if we decide to switch to another tool in the future (so we don't have to change the name everywhere again at that point).

gibfahn · 2017-09-10T15:40:20Z

make lint-js will do what make jslint currently does.

make lint-cpp will do what make cpplint currently does.

I'd rather we do this, it's not tool specific and it fits the rest of the makefile. I've also been confused by the use of jslint in our codebase.

Trott · 2017-09-10T16:04:16Z

What harm is there in introducing a warning right away?

Mostly people complaining. :-D There will undoubtedly be those who feel that the current task name should work indefinitely. If you're up for having that conversation sooner rather than later, then go for it.

My inclination is usually to make the smallest changes reasonable, one at a time. But that isn't always the best thing to do, so...
¯\(ツ)/¯

benjamingr · 2017-09-11T00:21:50Z

"jslint" is the name of a different tool, which can cause confusion.

Has it actually ever though? Has anyone tried to run the jslint tool on Node files because their make jslint failed?

ronkorving · 2017-09-11T01:24:29Z

@benjamingr That's what the PR description seems to describe.

I'm in favor of what @Trott is suggesting here. Make js and css postfixes. It fits the style, and it removes all ambiguity.

BridgeAR · 2017-09-11T03:33:48Z

@benjamingr when I set up my repo to open my first Node.js PRs I was very confused about the name.

I like make lint-js a lot as it does not comply to any library and I think that is a good way to make the live of newcomers a tad easier.

not-an-aardvark · 2017-09-11T03:36:27Z

I was also confused by the name at one point, although I wouldn't say my confusion actually caused any issues (since I never needed to know what the file was doing anyway).

thefourtheye · 2017-09-11T03:47:01Z

I am inclined to what @mscdex has said.

Trott · 2017-09-11T05:19:26Z

I think keeping 'jslint' is best, especially if we decide to switch to another tool in the future (so we don't have to change the name everywhere again at that point).

@mscdex @thefourtheye Given the reason, would lint-js and lint-cpp be acceptable? That would be analogous to test-addons etc.

mscdex · 2017-09-11T05:28:04Z

@Trott Those names themselves are fine, but I just don't see a need to change anything.

eljefedelrodeodeljefe · 2017-09-11T07:25:35Z

I am rather for keeping it. Let alone to avoid the churn and bikeshedding. Also don't feel there is a need.

cjihrig · 2017-09-11T13:01:07Z

I'm also in favor of keeping the existing names.

seishun · 2017-09-11T14:40:42Z

As suggested by @Trott, replaced eslint with lint-js and cpplint with lint-cpp. The old names still work, but print warnings.

This makes it more consistent with other task names such as test-addons and prevents confusion, so why not?

One thing I'm not sure about is vcbuild.bat output when running lint tasks. I wonder if "running eslint"/"running cpplint" would be preferable to "running lint-js"/"running lint-cpp".

refack · 2017-09-11T18:26:25Z

vcbuild.bat

-if not defined cpplint goto jslint
-call :run-cpplint src\*.c src\*.cc src\*.h test\addons\*.cc test\addons\*.h test\cctest\*.cc test\cctest\*.h test\gc\binding.cc tools\icu\*.cc tools\icu\*.h
+:lint-cpp
+if not defined lint-cpp goto lint-js


working env vars are named with _ so lint_cpp

refack · 2017-09-11T18:26:44Z

vcbuild.bat

 if defined enable_static goto exit
-if defined jslint_ci goto jslint-ci
-if not defined jslint goto exit
+if defined lint-js_ci goto lint-js-ci


lint_js_ci

refack · 2017-09-11T18:26:55Z

vcbuild.bat

-if defined jslint_ci goto jslint-ci
-if not defined jslint goto exit
+if defined lint-js_ci goto lint-js-ci
+if not defined lint-js goto exit


refack · 2017-09-11T18:30:50Z

vcbuild.bat

 if not exist tools\eslint\bin\eslint.js goto no-lint
-echo running jslint
+echo running lint-js
 %config%\node tools\eslint\bin\eslint.js --cache --rule "linebreak-style: 0" --rulesdir=tools\eslint-rules --ext=.js,.md benchmark doc lib test tools


To have a the output be similar to lint-cpp, I'd do:

set lint_js_cmd=%config%\node tools\eslint\bin\eslint.js --cache --rule "linebreak-style: 0" --rulesdir=tools\eslint-rules --ext=.js,.md benchmark doc lib test tools echo %lint_js_cmd% %lint_js_cmd% And similarly with `lint-js-ci`

refack · 2017-09-11T18:34:13Z

One thing I'm not sure about is vcbuild.bat output when running lint tasks. I wonder if "running eslint"/"running cpplint" would be preferable to "running lint-js"/"running lint-cpp".

So for lint-cpp the invocation trough call :run-python tools/lint-cpp.py %cppfilelist% will also output the actual CLI invocation of python, so the output is:

running lint-cpp 'src\*.c src\*.cc src\*.h test\addons\*.cc test\addons\*.h test\cctest\*.cc test\cctest\*.h test\gc\binding.cc tools\icu\*.cc tools\icu\*.h'
"C:\bin\dev\python27\python.exe" tools/lint-cpp.py  src\async-wrap.cc src\backtrace_posix.cc src\backtrace_win32 ...

Which IMHO is nice and useful
(tl;dr keep echo prompt as is, fix lint-js per my comments)

seishun · 2017-09-11T20:05:51Z

Which IMHO is nice and useful

Sorry, I messed up. The filename is actually cpplint.py, and it shouldn't be renamed because it's actually the name of the tool.

comments addressed

BridgeAR · 2017-09-13T18:17:20Z

I am a bit surprised that there are a couple of persons saying that there is no need for changing something when there were four people mentioning that they were confused about the name.
Especially when the old name is kept as a fallback as is it is right now. The confusion can lead to some unnecessary overhead for new people.

Does anyone of you @mscdex @cjihrig @thefourtheye @eljefedelrodeodeljefe have a strong opinion about this? I do not see any negative side as I do not see the churn as a real negative point here.

eljefedelrodeodeljefe · 2017-09-13T19:23:53Z

No strong opinion, but ideally I would want the understanding that churn and bikeshedding is distraction from doing valuable things. Not only the authors' but also those of multiple reviewers. And hence should be discouraged.

See, I learned to be conservative about the codebase since it's a very serious business running it in production.

Just think of this: what is the consequence in closing this now. Nothing really. Go ahead as you deem it pls.

refack · 2017-09-14T14:16:52Z

CI: https://ci.nodejs.org/job/node-test-commit/12361/

BridgeAR · 2017-09-19T20:20:33Z

So what is the conclusion here?

refack · 2017-09-19T20:30:58Z

AFAICT we have three +1 and two -0. Unless someone wants to explicitly -1, this could land.

aqrln

+1 from me too

BridgeAR · 2017-09-22T20:57:44Z

I think there was enough time to express a clear -1 and this has not happened, so it should be fine to land this.

@seishun it needs a rebase though.

"jslint" is the name of a tool that actually is not used, which can cause confusion. PR-URL: #15272 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

seishun · 2017-09-23T09:00:17Z

Running CI one last time since there was a rebase: https://ci.nodejs.org/job/node-test-pull-request/10217/

BridgeAR · 2017-09-24T15:32:59Z

Landed in b2eb987

"jslint" is the name of a tool that actually is not used, which can cause confusion. PR-URL: #15272 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

"jslint" is the name of a tool that actually is not used, which can cause confusion. PR-URL: nodejs/node#15272 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

MylesBorins · 2017-10-17T18:36:08Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

If it is backported I think it would make sense to remove the warning when calling make jslint, as that could be considered a behavior change in CI systems.

"jslint" is the name of a tool that actually is not used, which can cause confusion. Backport-PR-URL: #16297 PR-URL: #15272 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Sep 8, 2017

silverwind approved these changes Sep 8, 2017

View reviewed changes

Trott approved these changes Sep 8, 2017

View reviewed changes

Trott previously requested changes Sep 8, 2017

View reviewed changes

seishun changed the title ~~build: replace references to jslint with eslint~~ build: use generic names for linting tasks Sep 11, 2017

gibfahn approved these changes Sep 11, 2017

View reviewed changes

refack suggested changes Sep 11, 2017

View reviewed changes

refack approved these changes Sep 13, 2017

View reviewed changes

aqrln approved these changes Sep 19, 2017

View reviewed changes

BridgeAR closed this Sep 24, 2017

seishun deleted the eslint branch October 3, 2017 12:25

seishun restored the eslint branch October 3, 2017 12:25

MylesBorins added the backport-requested-v6.x label Oct 17, 2017

seishun mentioned this pull request Oct 18, 2017

[v6.x backport] build: use generic names for linting tasks #16297

Closed

2 tasks

MylesBorins mentioned this pull request Oct 19, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: use generic names for linting tasks #15272

build: use generic names for linting tasks #15272

seishun commented Sep 8, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

Trott left a comment •

edited

Loading

seishun commented Sep 9, 2017

Trott commented Sep 9, 2017

seishun commented Sep 10, 2017

mscdex commented Sep 10, 2017

gibfahn commented Sep 10, 2017

Trott commented Sep 10, 2017

benjamingr commented Sep 11, 2017

ronkorving commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

not-an-aardvark commented Sep 11, 2017

thefourtheye commented Sep 11, 2017

Trott commented Sep 11, 2017

mscdex commented Sep 11, 2017

eljefedelrodeodeljefe commented Sep 11, 2017

cjihrig commented Sep 11, 2017

seishun commented Sep 11, 2017 •

edited

Loading

refack Sep 11, 2017

refack Sep 11, 2017

refack Sep 11, 2017

refack Sep 11, 2017

refack commented Sep 11, 2017

seishun commented Sep 11, 2017

BridgeAR commented Sep 13, 2017 •

edited

Loading

eljefedelrodeodeljefe commented Sep 13, 2017

refack commented Sep 14, 2017

BridgeAR commented Sep 19, 2017

refack commented Sep 19, 2017

aqrln left a comment

BridgeAR commented Sep 22, 2017

seishun commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

MylesBorins commented Oct 17, 2017

build: use generic names for linting tasks #15272

build: use generic names for linting tasks #15272

Conversation

seishun commented Sep 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

seishun commented Sep 9, 2017

Trott commented Sep 9, 2017

seishun commented Sep 10, 2017

mscdex commented Sep 10, 2017

gibfahn commented Sep 10, 2017

Trott commented Sep 10, 2017

benjamingr commented Sep 11, 2017

ronkorving commented Sep 11, 2017

BridgeAR commented Sep 11, 2017

not-an-aardvark commented Sep 11, 2017

thefourtheye commented Sep 11, 2017

Trott commented Sep 11, 2017

mscdex commented Sep 11, 2017

eljefedelrodeodeljefe commented Sep 11, 2017

cjihrig commented Sep 11, 2017

seishun commented Sep 11, 2017 • edited Loading

refack Sep 11, 2017

Choose a reason for hiding this comment

refack Sep 11, 2017

Choose a reason for hiding this comment

refack Sep 11, 2017

Choose a reason for hiding this comment

refack Sep 11, 2017

Choose a reason for hiding this comment

refack commented Sep 11, 2017

seishun commented Sep 11, 2017

BridgeAR commented Sep 13, 2017 • edited Loading

eljefedelrodeodeljefe commented Sep 13, 2017

refack commented Sep 14, 2017

BridgeAR commented Sep 19, 2017

refack commented Sep 19, 2017

aqrln left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 22, 2017

seishun commented Sep 23, 2017

BridgeAR commented Sep 24, 2017

MylesBorins commented Oct 17, 2017

seishun commented Sep 8, 2017 •

edited

Loading

Trott left a comment •

edited

Loading

Trott left a comment •

edited

Loading

seishun commented Sep 11, 2017 •

edited

Loading

BridgeAR commented Sep 13, 2017 •

edited

Loading