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

tools: add debuglog eslint rule #32161

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 9, 2020

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

@BridgeAR BridgeAR requested review from jasnell and Trott 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
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2020
@Trott
Copy link
Member

Trott commented Mar 29, 2020

@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.

@jasnell
Copy link
Member

jasnell commented Mar 29, 2020

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented May 9, 2020

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

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2020
test/.eslintrc.yaml Outdated Show resolved Hide resolved
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.
These rules only apply for the test folder and will already be
checked for.
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR force-pushed the 2020-03-09-debuglog-eslint-rule branch from 5714228 to a5144ed Compare May 9, 2020 23:59
@BridgeAR
Copy link
Member Author

BridgeAR commented May 9, 2020

@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.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2020
@BridgeAR
Copy link
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.

@Trott
Copy link
Member

Trott commented May 10, 2020

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.)

@targos
Copy link
Member

targos commented May 10, 2020

Landed in c252f6c...1858472

targos pushed a commit that referenced this pull request May 10, 2020
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request May 10, 2020
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos closed this May 10, 2020
codebytere pushed a commit that referenced this pull request May 11, 2020
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
codebytere pushed a commit that referenced this pull request May 11, 2020
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@codebytere codebytere mentioned this pull request May 18, 2020
codebytere pushed a commit that referenced this pull request Jun 7, 2020
This makes sure all usages of `util.debuglog()` must contain the
string 'test' as argument.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
These rules only apply for the test folder and will already be
checked for.

PR-URL: #32161
Refs: #32078
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@codebytere codebytere mentioned this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants