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 — tools: add debuglog eslint rule by BridgeAR · Pull Request #32161 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented Mar 9, 2020

Copy link
Copy Markdown
Member

Please have a look at the commit messages for details.

Refs: #32078

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@BridgeAR
BridgeAR requested review from Trott and jasnell March 9, 2020 21:20
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 9, 2020
Trott
Trott approved these changes Mar 10, 2020
View reviewed changes
jasnell
jasnell approved these changes Mar 10, 2020
View reviewed changes
@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 12, 2020
trivikr
trivikr approved these changes Mar 18, 2020
View reviewed changes
@Trott

Trott commented Mar 29, 2020

Copy link
Copy Markdown
Member

@BridgeAR @jasnell For the long-term, what do you think about changing console.debug() so it only prints something when NODE_DEBUG is set? My reading of the spec is that this would be spec-compliant both in word and spirit.
All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell commented Mar 29, 2020

Copy link
Copy Markdown
Member

Not generally a fan of that approach given that we would lose the categorization that debuglog() provides. e.g. debuglog('test') vs debuglog('http2').
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
juanarbol
juanarbol approved these changes Apr 20, 2020
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 20, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 9, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

targos commented May 9, 2020

Copy link
Copy Markdown
Member 
14:57:47 not ok 21 - /home/iojs/build/workspace/node-test-linter/test/doctool/test-doctool-versions.js
14:57:47   ---
14:57:47   message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47   severity: error
14:57:47   data:
14:57:47     line: 32
14:57:47     column: 1
14:57:47     ruleId: no-restricted-syntax
14:57:47   messages:
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 35
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 36
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 41
14:57:47         column: 1
14:57:47         ruleId: no-restricted-syntax
14:57:47   ...
14:57:47 not ok 12 - /home/iojs/build/workspace/node-test-linter/test/parallel/test-macos-signed-deps.js
14:57:47   ---
14:57:47   message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47   severity: error
14:57:47   data:
14:57:47     line: 24
14:57:47     column: 3
14:57:47     ruleId: no-restricted-syntax
14:57:47   messages:
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 26
14:57:47         column: 3
14:57:47         ruleId: no-restricted-syntax
14:57:47     - message: Only use 'test' as debuglog value inside of the tests folder.
14:57:47       severity: error
14:57:47       data:
14:57:47         line: 27
14:57:47         column: 3
14:57:47         ruleId: no-restricted-syntax
14:57:47   ...
14:57:47 + exit 1
14:57:47 Build step 'Execute shell' marked build as failure
BridgeAR 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.
@targos targos removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 9, 2020
Trott
Trott reviewed May 9, 2020
View reviewed changes
Comment thread test/.eslintrc.yaml Resolved

Uh oh!

There was an error while loading. Please reload this page.
BridgeAR added 3 commits May 10, 2020 01:37
@BridgeAR
tools: add eslint rule to only pass through 'test' to debuglog
9b74eca 
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.
@BridgeAR
tools: remove obsolete no-restricted-syntax eslint rules
39e869d 
These rules only apply for the test folder and will already be
checked for.
@BridgeAR
fixup! tools: add eslint rule to only pass through 'test' to debuglog
a5144ed 
Signed-off-by: Ruben Bridgewater <[email protected]>
@BridgeAR
BridgeAR force-pushed the 2020-03-09-debuglog-eslint-rule branch from 5714228 to a5144ed Compare May 9, 2020 23:59
@BridgeAR

BridgeAR commented May 9, 2020

Copy link
Copy Markdown
Member Author

@targos thanks, I updated the rule only to match variable assignments from now on.

@Trott

For the long-term, what do you think about changing console.debug() so it only prints something when NODE_DEBUG is set? My reading of the spec is that this would be spec-compliant both in word and spirit.

I think that would be fine. Using debuglog(foo) would allow a more fine grained way of debugging while console.debug() would at least follow the actual "debug" intention the console name embodies.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 10, 2020
@BridgeAR

BridgeAR commented May 10, 2020

Copy link
Copy Markdown
Member Author

Note: I do not run a full CI on this, since the eslint rules are already run by our github actions and there's no other change involved.
All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented May 10, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Note: I do not run a full CI on this, since the eslint rules are already run by our github actions and there's no other change involved.

To whoever lands this: Please do a make lint-js before pushing to upstream master to check that something hasn't landed between the last GitHub Actions run and now that would get flagged by this new rule. (I'm pretty sure #33329 will not violate this rule, but that's an example of something that might if it lands before this gets merged.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos approved these changes May 10, 2020
View reviewed changes
@targos

targos commented May 10, 2020

Copy link
Copy Markdown
Member

Landed in c252f6c...1858472
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 10, 2020
@BridgeAR @targos
tools: add eslint rule to only pass through 'test' to debuglog
36b2e25 
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request May 10, 2020
@BridgeAR @targos
tools: remove obsolete no-restricted-syntax eslint rules
1858472 
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos closed this May 10, 2020
codebytere pushed a commit that referenced this pull request May 11, 2020
@BridgeAR @codebytere
tools: add eslint rule to only pass through 'test' to debuglog
804982c 
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this pull request May 11, 2020
@BridgeAR @codebytere
tools: remove obsolete no-restricted-syntax eslint rules
cf03fe5 
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
v14.3.0 proposal #33452
Merged
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@BridgeAR @codebytere
tools: add eslint rule to only pass through 'test' to debuglog
5d5e66c 
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@BridgeAR @codebytere
tools: remove obsolete no-restricted-syntax eslint rules
cdd7d3a 
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
v12.18.1 proposal #33811
Merged
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
@jasnell jasnell jasnell approved these changes
@trivikr trivikr trivikr approved these changes
@juanarbol juanarbol juanarbol approved these changes
+1 more reviewer
@targos targos targos 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. tools Issues and PRs related to the tools directory.

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

@BridgeAR @Trott @jasnell @nodejs-github-bot @targos @trivikr @juanarbol
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.