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

Use ESLint selectors in custom rules #17572

Merged
merged 4 commits into from
Dec 12, 2017
Merged

Use ESLint selectors in custom rules #17572

merged 4 commits into from
Dec 12, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 9, 2017

This PR simplifies several of our custom ESLint rules by using selector syntax instead of handwritten functions to identify matching AST nodes.

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

tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 9, 2017
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.

Great stuff! Just a couple of nits for legibility but not critical :)

}
}
// eslint-disable-next-line max-len
'ExpressionStatement[expression.type="CallExpression"][expression.callee.name="assert"][expression.arguments.0.type="BinaryExpression"]': function(node) {
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of legibility, we could extract this outside of module.exports and just use [astSelector]? Your call but I'm not a fan of the run-on line (having the [] beneath each other is a bit more legible IMHO).

}
// Catch common.mustCall(0)
// eslint-disable-next-line max-len
'CallExpression[callee.object.name="common"][callee.property.name="mustCall"][arguments.0.value=0]': report,
Copy link
Member

Choose a reason for hiding this comment

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

This could be extracted into a function outside of module.exports that takes input for the argument position, so something like getAstSelector(pos). Then we also don't need to have the run on line.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 11, 2017

Pulled selectors out into variables, as requested.

CI: https://ci.nodejs.org/job/node-test-pull-request/12036/

PR-URL: nodejs#17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: nodejs#17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: nodejs#17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
PR-URL: nodejs#17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@cjihrig cjihrig merged commit 746534b into nodejs:master Dec 12, 2017
@cjihrig cjihrig deleted the lint branch December 12, 2017 02:06
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Missing landed in

Landed in e4a7109 66d3e6b 0c1dee5 746534b

gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Didn't cherry-pick the last commit back to 6.x as it conflicted.

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #17572
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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.

5 participants