Skip to content

Conversation

@gibfahn
Copy link
Member

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-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 27, 2017
@gibfahn
Copy link
MemberAuthor

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;fiif [ !-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/ >[email protected] 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
Contributor

@apapirovskiapapirovski 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
MemberAuthor

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!

@gibfahngibfahn mentioned this pull request Oct 27, 2017
2 tasks
@gibfahn
Copy link
MemberAuthor

@gibfahngibfahn mentioned this pull request Oct 31, 2017
2 tasks
@gibfahn
Copy link
MemberAuthor

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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
@gibfahngibfahn merged commit 60d055e into nodejs:masterNov 2, 2017
@gibfahngibfahn 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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
@cjihrigcjihrig 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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
@gibfahngibfahn mentioned this pull request Nov 21, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@gibfahn@Florian-R@apapirovski@jasnell@watilde@cjihrig@XadillaX@joyeecheung@nodejs-github-bot