Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc, tools: doc linting does not work in CI #12635

Closed
vsemozhetbyt opened this issue Apr 25, 2017 · 7 comments
Closed

doc, tools: doc linting does not work in CI #12635

vsemozhetbyt opened this issue Apr 25, 2017 · 7 comments
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 25, 2017

  • Subsystem: doc, tools

It seems #12563 has not made all needed changes in build scripts to run doc linting on CI.

I've tried to skim PRs from this search: is:pr is:open label:doc -label:stalled and run linter CI on them.

See this inconsistency: #12549 (comment)

It seems, CI uses commands not addressed in the #12563 (see https://ci.nodejs.org/job/node-test-linter/8536/console).

What should be added?

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 25, 2017
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

It seems these blocks also need to be edited, but I don't know how exactly:

node/Makefile

Lines 861 to 864 in b4fea2a

jslint-ci:
@echo "Running JS linter..."
$(NODE) tools/jslint.js $(PARALLEL_ARGS) -f tap -o test-eslint.tap \
benchmark lib test tools

node/vcbuild.bat

Lines 427 to 430 in b4fea2a

:jslint-ci
echo running jslint-ci
%config%\node tools\jslint.js -J -f tap -o test-eslint.tap benchmark lib test tools
goto exit

Maybe some other files are also concerned (tools/jslint.js ?).

сс @not-an-aardvark, @silverwind, @Trott

@Trott
Copy link
Member

Trott commented Apr 25, 2017

@vsemozhetbyt I think all you need to do is add doc to the list of directories being linted. They're in alphabetical order, so it would go between benchmark and lib. Basically, compare line 864 of the Makefile (the version you have linked above) with line 859. Similar change in vcbuild.bat.

@Trott
Copy link
Member

Trott commented Apr 25, 2017

@vsemozhetbyt Oh, wait, yeah, you'll also need to add the --ext=.js,.md stuff. And that will involve editing tools/jslint.js (which was originally authored by @mscdex, who I'll @-mention just in case there's already a provision for adding arbitrary CLI flags or something).

@not-an-aardvark
Copy link
Contributor

Oh, I never realized that jslint.js was a runner for ESLint. I always thought it was running JSLint.

@Trott
Copy link
Member

Trott commented Apr 25, 2017

@not-an-aardvark With make jslint, we run eslint with caching because people are likely to run that multiple times. On my machine, the first run takes about 23 seconds, but subsequent runs (once the cache is populated) are less than 2 seconds. Sweet!

But on CI, there is no second run. There is only the first run. So jslint.js is used to parallelize the linting a bit to make it faster. On my machine, it takes about 15 seconds to run. It might be slightly longer once docs are added to its lint load the way they are for make jslint.

Given that the linter is the fastest CI job we have, this is literally marginal value, but it used to run everywhere before ESLint had caching (or maybe before we knew to turn caching on).

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2017

@Trott @not-an-aardvark It seems we need to update cliOptions for CLIEngine with extensions property. I shall try a PR.

@vsemozhetbyt
Copy link
Contributor Author

Trying: #12640

evanlucas pushed a commit that referenced this issue May 1, 2017
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this issue May 2, 2017
PR-URL: #12640
Fixes: #12635
Refs: #12563
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

No branches or pull requests

3 participants