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.

Sign up for GitHub

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

Jump to bottom

make lint-ci lints docs, but make lint doesn't #18466

Closed
evanlucas opened this issue Jan 30, 2018 · 3 comments
Closed

make lint-ci lints docs, but make lint doesn't #18466

evanlucas opened this issue Jan 30, 2018 · 3 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory.

Comments

@evanlucas
Copy link
Contributor

After working on the v9.5.0 release proposal, I was concerned to see that the node-test-linter ci job failed. Apparently, we are linting docs in CI, but not locally with make lint.

IMO, if we lint in CI, we should lint locally. Otherwise, people could be confused as to why it passes locally but not when run in CI. Would love to hear the opinions of others
@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jan 30, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jan 30, 2018 •
edited
Loading

If I get it right, there is no doc linting on Windows, in vcbuild.bat (as well as we do not build docs and do not test doc building on Windows). If it is true and we add doc linting locally, we need to document this restriction.

@joyeecheung joyeecheung added the good first issue Issues that are suitable for first-time contributors. label Jan 31, 2018
@joyeecheung
Copy link
Member

joyeecheung commented Jan 31, 2018 •
edited
Loading

Ironically, the documentation of make lint is:

node/Makefile

Line 1181 in 0993fbe

lint: ## Run JS, C++, MD and doc linters.

Labeling this as a good first issue. To fix it, add another line in target mentioned above so that make lint runs lint-md, just like what lint-ci does. I am happy to mentor if anyone wants one.

@camilo86
Copy link
Contributor

I can get on that right now. First time contributing to nodejs

@camilo86 camilo86 mentioned this issue Jan 31, 2018
Closed
1 task
camilo86 added a commit to camilo86/node that referenced this issue Jan 31, 2018
@camilo86
build: added doc linting when runnning make lint
f97b414 
Fixes: nodejs#18466
@targos targos closed this as completed in 0548034 Feb 6, 2018
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@camilo86 @MylesBorins
build: add doc linting when runnning make lint
529f42c 
Fixes: #18466

PR-URL: #18472
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@camilo86 @MylesBorins
build: add doc linting when runnning make lint
a19fd95 
Fixes: #18466

PR-URL: #18472
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 21, 2018
@camilo86 @MylesBorins
build: add doc linting when runnning make lint
c642e22 
Fixes: #18466

PR-URL: #18472
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
gibfahn pushed a commit that referenced this issue Apr 13, 2018
@camilo86 @gibfahn
build: add doc linting when runnning make lint
7b6d93c 
Fixes: #18466

PR-URL: #18472
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@camilo86 @MayaLekova
build: add doc linting when runnning make lint
a83cd47 
Fixes: nodejs#18466

PR-URL: nodejs#18472
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@evanlucas @camilo86 @joyeecheung @vsemozhetbyt