GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: recommend test-doc instead of lint-md by aduh95 · Pull Request #35708 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Oct 19, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to Node.js builds or CI infrastructure. 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

Trott commented Oct 19, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Oct 19, 2020
View reviewed changes
Comment thread Makefile Outdated

@Trott Trott Oct 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aduh95 aduh95 Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@codecov-io

This comment has been minimized.

Sign in to view
Trott
Trott approved these changes Oct 19, 2020
View reviewed changes
@aduh95

aduh95 commented Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 19, 2020
@Trott

Trott commented Oct 19, 2020

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 19, 2020
@nodejs-github-bot

nodejs-github-bot commented Oct 19, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Oct 19, 2020

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Oct 19, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Oct 19, 2020

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

No you're right, I was wrong: checkLinks.js can be moved to linter indeed, the linter does indeed check all those files.
richardlau reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

@nodejs/platform-windows friendly ping
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson approved these changes Oct 23, 2020
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95 aduh95 added review wanted PRs that need review. windows Issues and PRs related to the Windows platform. labels Oct 25, 2020
aymen94
aymen94 approved these changes Oct 27, 2020
View reviewed changes
@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

aduh95 commented Oct 27, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 27, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Oct 28, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95 added a commit to aduh95/node that referenced this pull request Nov 2, 2020
@aduh95
build: fix lint-js-fix target
1be70e9 
Refs: nodejs#35708
@aduh95 aduh95 mentioned this pull request Nov 2, 2020
build: fix lint-js-fix target #35927
Closed
2 tasks done
nodejs-github-bot pushed a commit that referenced this pull request Nov 2, 2020
@aduh95 @nodejs-github-bot
build: fix lint-js-fix target
bf6883d 
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
@aduh95 @targos
build,tools: add lint-js-doc target
273e147 
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
@aduh95 @targos
build: add vcbuilt test-doc target
a5fa849 
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
@aduh95 @targos
doc: recommend test-doc instead of lint-md
8c2b179 
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
@aduh95 @targos
build: fix lint-js-fix target
b11c737 
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
v15.1.0 release proposal #35948
Merged
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
@aduh95 @BethGriggs
build,tools: add lint-js-doc target
8e6029d 
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
@aduh95 @BethGriggs
build: add vcbuilt test-doc target
d1a6ce2 
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
@aduh95 @BethGriggs
doc: recommend test-doc instead of lint-md
a618d79 
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
@aduh95 @BethGriggs
build: fix lint-js-fix target
a389a72 
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
@aduh95 @BethGriggs
build,tools: add lint-js-doc target
1fdcdce 
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
@aduh95 @BethGriggs
build: add vcbuilt test-doc target
d6be20f 
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
@aduh95 @BethGriggs
doc: recommend test-doc instead of lint-md
0ea5649 
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
@aduh95 @BethGriggs
build: fix lint-js-fix target
a01802d 
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
v14.15.2 proposal #36476
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@aduh95 @BethGriggs
build,tools: add lint-js-doc target
77b7c98 
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
@aduh95 @BethGriggs
build: add vcbuilt test-doc target
67d3182 
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
@aduh95 @BethGriggs
doc: recommend test-doc instead of lint-md
3f3d2d7 
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
@aduh95 @BethGriggs
build: fix lint-js-fix target
f33fa26 
Refs: #35708

PR-URL: #35927
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@Renegade334 Renegade334 mentioned this pull request Jun 6, 2025
build: lint under make test? #58606
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
+2 more reviewers
@mhdawson mhdawson mhdawson approved these changes
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. review wanted PRs that need review. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@aduh95 @Trott @codecov-io @nodejs-github-bot @richardlau @mhdawson @aymen94
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.