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: auto fix custom eslint rule for lowercase-name-for-primitive.js #17715

Conversation

shobhitchittora
Copy link
Contributor

@shobhitchittora shobhitchittora commented Dec 17, 2017

Refs: #16636

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
  • modify tests

Affected core subsystem(s)

Tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 17, 2017
@shobhitchittora
Copy link
Contributor Author

Hey @apapirovski @BridgeAR .
Please review this.

@shobhitchittora shobhitchittora force-pushed the lowercase-name-for-primitive-fixer branch from 8654800 to 594ae58 Compare December 17, 2017 10:49
@@ -35,9 +35,20 @@ module.exports = function(context) {
function checkName(node, name) {
const lowercaseName = name.toLowerCase();
if (primitives.includes(lowercaseName) && !primitives.includes(name)) {
console.log(lowercaseName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the console.log()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah forgot it was there! Removed 👍🏻.

@shobhitchittora shobhitchittora force-pushed the lowercase-name-for-primitive-fixer branch from 594ae58 to ece39e7 Compare December 17, 2017 17:24
@Trott
Copy link
Member

Trott commented Dec 17, 2017

Do we usually have tests for fixer functions? /cc @not-an-aardvark

@shobhitchittora
Copy link
Contributor Author

@Trott not afaik.

Trott
Trott previously approved these changes Dec 17, 2017
@Trott
Copy link
Member

Trott commented Dec 17, 2017

@shobhitchittora Looks good to me. Thanks for the pull request!

1. Fixer for lowercase-name-for-primitive.js
2. Modified the node passed to fix it.

Refs : nodejs#16636
@shobhitchittora shobhitchittora force-pushed the lowercase-name-for-primitive-fixer branch from ece39e7 to a389548 Compare December 17, 2017 17:53
@starkwang
Copy link
Contributor

starkwang commented Dec 18, 2017

Hi @shobhitchittora ! Thank you for working on this!

Could you expand the test in /test/parallel/test-eslint-lowercase-name-for-primitive.js for the auto fixer ? (simply add the output prop in invalid array)

Here is an example to see how it works: /test/parallel/test-eslint-no-let-in-for-declaration.js

@Trott Trott dismissed their stale review December 18, 2017 03:50

Happy to approve again after @strakwang's requested test is implemented.

@shobhitchittora
Copy link
Contributor Author

@starkwang Thanks for the heads up.
@Trott extended the tests.

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.

Seems good but a bit of clean up would be nice.

names.elements.forEach((name) => {
checkName(node, name.value);
names.elements.forEach((name, index) => {
checkName(names.elements[index], name.value);
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this just be name instead of names.elements[index]? Pretty sure we can also get rid of the second argument here and above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏻

@@ -36,8 +36,18 @@ module.exports = function(context) {
const lowercaseName = name.toLowerCase();
if (primitives.includes(lowercaseName) && !primitives.includes(name)) {
Copy link
Member

@apapirovski apapirovski Dec 18, 2017

Choose a reason for hiding this comment

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

name could just be replaced with node.value now.

Also, pretty sure !primitives.includes(name) could be simplified to name !== lowercaseName (and it could be put as the first condition so the includes only runs if necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes.

@apapirovski
Copy link
Member

@apapirovski
Copy link
Member

@@ -23,19 +23,23 @@ new RuleTester().run('lowercase-name-for-primitive', rule, {
invalid: [
{
code: 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "a", "Number")',
errors: [{ message: 'primitive should use lowercase: Number' }]
errors: [{ message: 'primitive should use lowercase: Number' }],
output: 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "a", "number")'
Copy link
Contributor

Choose a reason for hiding this comment

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

CI failed due to a small problem:
The input code and expected output are using double quotes ", but the autofixer will change it to single quotes, which makes the actual output cannot match the expected output.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shobhitchittora Could you change these double quotes in code and output to using single quotes?

Copy link
Contributor Author

@shobhitchittora shobhitchittora Dec 25, 2017

Choose a reason for hiding this comment

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

@starkwang pushed a fix. How did you check the problem in Jenkins that the output is not correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I modified some codes in eslint (eslint/lib/testers/rule-tester.js) to expose the error message.
You can see eslint/eslint#9769 for more detail. 😉

@starkwang
Copy link
Contributor

I also send a PR to eslint for more detailed assert message: eslint/eslint#9769

@starkwang
Copy link
Contributor

@starkwang
Copy link
Contributor

The CI failed again:

Received:
'new errors.TypeError(\'ERR_INVALID_ARG_TYPE\', \'a\',\n            \'number\')'

Expected:
'new errors.TypeError(\'ERR_INVALID_ARG_TYPE\', \'a\',\n              \'number\')'

code: 'new errors.TypeError("ERR_INVALID_ARG_TYPE", "a", "Number")',
errors: [{ message: 'primitive should use lowercase: Number' }]
code: `new errors.TypeError('ERR_INVALID_ARG_TYPE', 'a',
'Number')`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@shobhitchittora Could you change these backquote to singlequote? For example:

{
  code: 'new errors.TypeError(\'ERR_INVALID_ARG_TYPE\', \'a\', ' +
        '\'Number\''
}

In addition, you can use this command to check if your change will pass CI:

node test/parallel/test-eslint-lowercase-name-for-primitive.js 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and started a build.

1. Removes template string
2. Uses escaped quote.

Refs : nodejs#16636
@starkwang
Copy link
Contributor

@starkwang
Copy link
Contributor

Landed in 639770c 💯 🥇

@starkwang starkwang closed this Dec 26, 2017
starkwang pushed a commit that referenced this pull request Dec 26, 2017
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@shobhitchittora shobhitchittora deleted the lowercase-name-for-primitive-fixer branch December 26, 2017 07:14
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
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.

7 participants