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: recommend test-doc instead of lint-md #35708

Merged
merged 3 commits into from
Oct 30, 2020

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 19, 2020

The documentation style guide used to recommend checking changes in the docs by running make lint-md. This leaves out some important checks which are contained in the test-doc build target.

This also adds a lint-js-doc target, which lints only Markdown files. This is to make test-doc tests doc files only.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

cc @nodejs/documentation

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 19, 2020
@aduh95 aduh95 changed the title Recommand test doc rather than lint md doc: recommend test-doc instead of lint-md Oct 19, 2020
@aduh95 aduh95 force-pushed the recommand-test-doc-rather-than-lint-md branch from af8e5cb to f51ad26 Compare October 19, 2020 11:06
@Trott
Copy link
Member

Trott commented Oct 19, 2020

There's no equivalent vcbuild test-doc for users. Do you want to try to add that to vcbuild.bat?

# Note that on the CI `lint-js-ci` is run instead.
# Lints the JavaScript code with eslint.
lint-js:
lint-js: EXTENSIONS=.js,.mjs,.md
lint-js-doc: EXTENSIONS=.md
Copy link
Member

Choose a reason for hiding this comment

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

Will this bust the ESLint cache and make the next run of lint-js take (typically) around 30 seconds instead of 3 seconds? If so, might we want to just lint-js to keep the ESLint cache intact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem so:

$ make lint-js && shasum .eslintcache
Running JS linter...
88f96b5b75f52cd813f327e329b5b90ae977e3cc  .eslintcache
$ make lint-js-doc && shasum .eslintcache
Running JS linter...
88f96b5b75f52cd813f327e329b5b90ae977e3cc  .eslintcache

@codecov-io

This comment has been minimized.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 19, 2020

There's no equivalent vcbuild test-doc for users. Do you want to try to add that to vcbuild.bat?

I've tried to add test-doc in the vcbuild.bat file. I'm not familiar with the syntax, and I haven't tested it, so by all means please review 😅

I haven't added a vcbuild lint-js-doc target, I couldn't find an elegant way of implementing it.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@Trott
Copy link
Member

Trott commented Oct 19, 2020

I've tried to add test-doc in the vcbuild.bat file. I'm not familiar with the syntax, and I haven't tested it, so by all means please review 😅

@nodejs/platform-windows

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

The documentation style guide used to recommend checking changes in the docs by running make lint-md. This leaves out some important checks which are contained in the test-doc build target.

Non-blocking for this PR, but I've always wondered why the things checkLinks.js does wasn't implemented as a markdown linter plugin/rule.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 19, 2020

Non-blocking for this PR, but I've always wondered why the things checkLinks.js does wasn't implemented as a markdown linter plugin/rule.

@richardlau originally checkLinks was introduced to check all markdown files that was not covered by the linter (guides, README, CODE_OF_CONDUCT, etc.). It has been recently modified to also check docs, so it is a bit stepping on the toes of the linter, but I think we still want to check links in all those non-doc markdown files. The reason for it was the docs used to use .html extension to link to markdown files. It has been recently modified use .md so now this restriction doesn't exist anymore.

I guess we could mode the command to lint-md though, but I'd prefer not to do that in this PR.

@richardlau
Copy link
Member

Non-blocking for this PR, but I've always wondered why the things checkLinks.js does wasn't implemented as a markdown linter plugin/rule.

@richardlau originally checkLinks was introduced to check all markdown files that was not covered by the linter (guides, README, CODE_OF_CONDUCT, etc.). It has been recently modified to also check docs, so it is a bit stepping on the toes of the linter, but I think we still want to check links in all those non-doc markdown files.

I guess we could mode the command to lint-md though, but I'd prefer not to do that in this PR.

I'm confused. I thought lint-md lints both API docs and the other non-API markdown files (like the README).

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 19, 2020

No you're right, I was wrong: checkLinks.js can be moved to linter indeed, the linter does indeed check all those files.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 22, 2020

@nodejs/platform-windows friendly ping

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@aduh95 aduh95 added review wanted PRs that need reviews. windows Issues and PRs related to the Windows platform. labels Oct 25, 2020
@aduh95 aduh95 force-pushed the recommand-test-doc-rather-than-lint-md branch 2 times, most recently from 3b0c574 to cc2d361 Compare October 27, 2020 19:15
@aduh95
Copy link
Contributor Author

aduh95 commented Oct 27, 2020

vcbuild.bat test-doc seems to indeed run the doc-related tests (https://github.com/nodejs/node/pull/35708/checks?check_run_id=1315987001) and fails when it needs to (https://github.com/nodejs/node/pull/35708/checks?check_run_id=1316526572). I've reordered the comits, I think it's ready to land now.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 27, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 deleted the recommand-test-doc-rather-than-lint-md branch October 30, 2020 11:03
aduh95 added a commit to aduh95/node that referenced this pull request Nov 2, 2020
@aduh95 aduh95 mentioned this pull request Nov 2, 2020
2 tasks
nodejs-github-bot pushed a commit that referenced this pull request Nov 2, 2020
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
Add a build target to lint JS code in Markdown files only.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
The documentation style guide used to recommend checking changes in the
docs by running `make lint-md`. This leaves out some important checks
which are contained in the `test-doc` build target. This commit also
replaces `lint` by `lint-md` in the list of `test-doc`'s prerequisites.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targos targos mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Add a build target to lint JS code in Markdown files only.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
The documentation style guide used to recommend checking changes in the
docs by running `make lint-md`. This leaves out some important checks
which are contained in the `test-doc` build target. This commit also
replaces `lint` by `lint-md` in the list of `test-doc`'s prerequisites.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Add a build target to lint JS code in Markdown files only.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
The documentation style guide used to recommend checking changes in the
docs by running `make lint-md`. This leaves out some important checks
which are contained in the `test-doc` build target. This commit also
replaces `lint` by `lint-md` in the list of `test-doc`'s prerequisites.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Add a build target to lint JS code in Markdown files only.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
The documentation style guide used to recommend checking changes in the
docs by running `make lint-md`. This leaves out some important checks
which are contained in the `test-doc` build target. This commit also
replaces `lint` by `lint-md` in the list of `test-doc`'s prerequisites.

PR-URL: #35708
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. review wanted PRs that need reviews. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants