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: auto fix custom eslint rule for lowercase-name-for-primitive.js by shobhitchittora · Pull Request #17715 · nodejs/node

Conversation

@shobhitchittora

@shobhitchittora shobhitchittora commented Dec 17, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Refs: #16636

Checklist

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

Affected core subsystem(s)

Tools

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 17, 2017
@shobhitchittora

shobhitchittora commented Dec 17, 2017

Copy link
Copy Markdown
Contributor Author

Hey @apapirovski @BridgeAR .
Please review this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shobhitchittora
shobhitchittora force-pushed the lowercase-name-for-primitive-fixer branch from 8654800 to 594ae58 Compare December 17, 2017 10:49
cjihrig
cjihrig reviewed Dec 17, 2017
View reviewed changes
Comment thread tools/eslint-rules/lowercase-name-for-primitive.js Outdated

@cjihrig cjihrig Dec 17, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

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

@shobhitchittora shobhitchittora Dec 17, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 👍🏻.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shobhitchittora
shobhitchittora force-pushed the lowercase-name-for-primitive-fixer branch from 594ae58 to ece39e7 Compare December 17, 2017 17:24
@Trott

Trott commented Dec 17, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

shobhitchittora commented Dec 17, 2017

Copy link
Copy Markdown
Contributor Author

@Trott not afaik.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott previously approved these changes Dec 17, 2017
View reviewed changes
@Trott

Trott commented Dec 17, 2017

Copy link
Copy Markdown
Member

@shobhitchittora Looks good to me. Thanks for the pull request!
shobhitchittora reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shobhitchittora
tools: auto fix custom eslint rule for lowercase-name-for-primitive.js
a389548 
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

starkwang commented Dec 18, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott dismissed their stale review December 18, 2017 03:50

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

@shobhitchittora
tools: auto fix custom eslint rule for lowercase-name-for-primitive.js
b2d1f5b 
1. Extends test cases with output scenarios.

Refs : nodejs#16636
@shobhitchittora

shobhitchittora commented Dec 18, 2017

Copy link
Copy Markdown
Contributor Author

@starkwang Thanks for the heads up.
@Trott extended the tests.
Trott 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.
apapirovski
apapirovski approved these changes Dec 18, 2017
View reviewed changes

@apapirovski apapirovski left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/eslint-rules/lowercase-name-for-primitive.js Outdated
names.elements.forEach((name) => {
checkName(node, name.value);
names.elements.forEach((name, index) => {
checkName(names.elements[index], name.value);

@apapirovski apapirovski Dec 18, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

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

@shobhitchittora shobhitchittora Dec 19, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍🏻

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/eslint-rules/lowercase-name-for-primitive.js Outdated
@@ -36,8 +36,18 @@ module.exports = function(context) {
const lowercaseName = name.toLowerCase();
if (primitives.includes(lowercaseName) && !primitives.includes(name)) {

@apapirovski apapirovski Dec 18, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

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

@shobhitchittora shobhitchittora Dec 19, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Made the changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@apapirovski

apapirovski commented Dec 18, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shobhitchittora
tools: auto fix custom eslint rule for lowercase-name-for-primitive.js
42009c2 
1. Changes as per review comments.

Refs : nodejs#16636
@apapirovski

apapirovski commented Dec 24, 2017

Copy link
Copy Markdown
Contributor

CI to merge: https://ci.nodejs.org/job/node-test-pull-request-lite/42/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
starkwang
starkwang reviewed Dec 25, 2017
View reviewed changes
Comment thread test/parallel/test-eslint-lowercase-name-for-primitive.js Outdated
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")'

@starkwang starkwang Dec 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

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

@starkwang starkwang Dec 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

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

@shobhitchittora shobhitchittora Dec 25, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

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

@starkwang starkwang Dec 25, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@starkwang

starkwang commented Dec 25, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@shobhitchittora
tools: auto fix custom eslint rule for lowercase-name-for-primitive.js
1799ec3 
1. Fixed test with proper quotes.

Refs : nodejs#16636
@starkwang

starkwang commented Dec 26, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

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

starkwang commented Dec 26, 2017

Copy link
Copy Markdown
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\')'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
starkwang
starkwang reviewed Dec 26, 2017
View reviewed changes
Comment thread test/parallel/test-eslint-lowercase-name-for-primitive.js Outdated
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')`,

@starkwang starkwang Dec 26, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

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

@shobhitchittora shobhitchittora Dec 26, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shobhitchittora
tools: auto fix custom eslint rule for lowercase-name-for-primitive.js
a23ca88 
1. Removes template string
2. Uses escaped quote.

Refs : nodejs#16636
@starkwang

starkwang commented Dec 26, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
starkwang
starkwang approved these changes Dec 26, 2017
View reviewed changes
@starkwang

starkwang commented Dec 26, 2017

Copy link
Copy Markdown
Contributor

Landed in 639770c 💯 🥇
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@starkwang starkwang closed this Dec 26, 2017
starkwang pushed a commit that referenced this pull request Dec 26, 2017
@shobhitchittora @starkwang
tools: autofixer for lowercase-name-for-primitive
639770c 
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
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
034c998 
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
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
ce25443 
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
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
2d07243 
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
v9.4.0 proposal #18069
Merged
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@shobhitchittora @gibfahn
tools: autofixer for lowercase-name-for-primitive
2ed63d1 
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
@shobhitchittora @gibfahn
tools: autofixer for lowercase-name-for-primitive
eee6cd0 
PR-URL: #17715
Refs: #16636
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@gibfahn gibfahn added land-on-v6.x labels Jan 24, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@shobhitchittora @gibfahn
tools: autofixer for lowercase-name-for-primitive
f88afb4 
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
v6.13.0 proposal #18342
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
7790887 
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
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
b9c6922 
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
@shobhitchittora @MylesBorins
tools: autofixer for lowercase-name-for-primitive
6187aec 
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

Reviewers

@cjihrig cjihrig cjihrig left review comments
@Trott Trott Trott left review comments
+2 more reviewers
@apapirovski apapirovski apapirovski approved these changes
@starkwang starkwang starkwang 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

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

@shobhitchittora @Trott @starkwang @apapirovski @cjihrig @gibfahn @nodejs-github-bot
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.