doc: recommend test-doc instead of lint-md by aduh95 · Pull Request #35708 · nodejs/node · GitHub

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

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

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?

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

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.

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

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

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.

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.

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

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

@nodejs/platform-windows friendly ping

LGTM

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.

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

Refs: nodejs#35708