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

build: suppress lint-md output #16551

Merged
merged 1 commit into from
Nov 2, 2017
Merged

build: suppress lint-md output #16551

merged 1 commit into from
Nov 2, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Oct 27, 2017

We don't need to print out the output if we've already installed it, at
the same time we do want to see some output when we haven't installed.

Checklist
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 27, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017

Previous output after installing remark:

▶ make lint                                                                                                                                                                ~/wrk/com/DANGER/node (master)
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
	  benchmark doc lib test tools
Running C++ linter...
Total errors found: 0
if [ ! -d tools/remark-cli/node_modules ]; then \
		cd tools/remark-cli && ../.././node ../.././deps/npm/bin/npm-cli.js install; fi
if [ ! -d tools/remark-preset-lint-node/node_modules ]; then \
		cd tools/remark-preset-lint-node && ../.././node ../.././deps/npm/bin/npm-cli.js install; fi
Running Markdown linter...
./node tools/remark-cli/cli.js -q -f \
		./*.md doc src lib benchmark tools/doc/ tools/icu/

New output after installing remark

▶ make lint                                                                                                                                                                  ~/wrk/com/node (quiet-lint*)
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
	  benchmark doc lib test tools
Running C++ linter...
Total errors found: 0
Running Markdown linter...
./node tools/remark-cli/cli.js -q -f \
		./*.md doc src lib benchmark tools/doc/ tools/icu/

Output when you install remark

Markdown linter: installing remark-cli into tools/

> fsevents@1.1.2 install /Users/gib/wrk/com/node/tools/remark-cli/node_modules/fsevents
> node install

[... lots of npm output here ...]

added 272 packages in 8.462s
Markdown linter: installing remark-preset-lint-node into tools/
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN You are using a pre-release version of node and things may not work as expected

added 48 packages in 2.016s
Running Markdown linter...
./node tools/remark-cli/cli.js -q -f \
		./*.md doc src lib benchmark tools/doc/ tools/icu/
make: *** [lint] Error 2

I think keeping the npm install output is important.

@Florian-R
Copy link

Dupe of #16508 no?

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

I slightly prefer this one, or if @danbev modifies theirs to match. I don't think it should be made completely silent.

@gibfahn
Copy link
Member Author

gibfahn commented Oct 27, 2017

Dupe of #16508 no?

Damn, I knew I should have gone through open PRs first.

@danbev feel free to steal code, yours already has more reviews!

@gibfahn gibfahn mentioned this pull request Oct 27, 2017
2 tasks
@gibfahn
Copy link
Member Author

gibfahn commented Oct 30, 2017

@gibfahn gibfahn mentioned this pull request Oct 31, 2017
2 tasks
@gibfahn
Copy link
Member Author

gibfahn commented Oct 31, 2017

Better fix at #16635, will close if that lands.

We don't need to print out the output if we've already installed it, at
the same time we do want to see some output when we haven't installed.

PR-URL: nodejs#16551
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@gibfahn gibfahn merged commit 60d055e into nodejs:master Nov 2, 2017
@gibfahn gibfahn deleted the quiet-lint branch November 2, 2017 22:36
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
We don't need to print out the output if we've already installed it, at
the same time we do want to see some output when we haven't installed.

PR-URL: nodejs#16551
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
gibfahn added a commit that referenced this pull request Nov 14, 2017
We don't need to print out the output if we've already installed it, at
the same time we do want to see some output when we haven't installed.

PR-URL: #16551
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants