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: fix inspector-check reporting #16902

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 9, 2017

Currently the inspector-check rule does not store the node when the
usage of a require('inspector') is done. This means that it is not
possible to disable this rule. For example, if you add the following to
a test that uses the inspector module:

//common.skipIfInspectorDisabled();
/* eslint-disable inspector-check */

This will not disable the check and there will still be a lint error
reported.

This commit stores the node where the require statement is done which
allows for the rule to also be disabled.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

Currently the inspector-check rule does not store the node when the
usage of a require('inspector') is done. This means that it is not
possible to disable this rule. For example, if you add the following to
a test that uses the inspector module:
//common.skipIfInspectorDisabled();
/* eslint-disable inspector-check */

This will not disable the check and there will still be a lint error
reported.

This commit stores the node where the require statement is done which
allows for the rule to also be disabled.
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 9, 2017
@danbev
Copy link
Contributor Author

danbev commented Nov 9, 2017

@@ -30,8 +30,10 @@ module.exports = function(context) {
}

function reportIfMissing(context, node) {
if (usesInspector && !hasInspectorCheck) {
context.report(node, msg);
if (missingCheckNodes.length > 0 && !hasInspectorCheck) {
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to check missingCheckNodes.length > 0, just the !hasInspectorCheck is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I'll fix that. Thanks

@danbev
Copy link
Contributor Author

danbev commented Nov 13, 2017

Landed in c2d63a9

@danbev danbev closed this Nov 13, 2017
danbev added a commit that referenced this pull request Nov 13, 2017
Currently the inspector-check rule does not store the node when the
usage of a require('inspector') is done. This means that it is not
possible to disable this rule. For example, if you add the following to
a test that uses the inspector module:
//common.skipIfInspectorDisabled();
/* eslint-disable inspector-check */

This will not disable the check and there will still be a lint error
reported.

This commit stores the node where the require statement is done which
allows for the rule to also be disabled.

PR-URL: #16902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Currently the inspector-check rule does not store the node when the
usage of a require('inspector') is done. This means that it is not
possible to disable this rule. For example, if you add the following to
a test that uses the inspector module:
//common.skipIfInspectorDisabled();
/* eslint-disable inspector-check */

This will not disable the check and there will still be a lint error
reported.

This commit stores the node where the require statement is done which
allows for the rule to also be disabled.

PR-URL: #16902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
Currently the inspector-check rule does not store the node when the
usage of a require('inspector') is done. This means that it is not
possible to disable this rule. For example, if you add the following to
a test that uses the inspector module:
//common.skipIfInspectorDisabled();
/* eslint-disable inspector-check */

This will not disable the check and there will still be a lint error
reported.

This commit stores the node where the require statement is done which
allows for the rule to also be disabled.

PR-URL: #16902
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@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
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants