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.

Sign up for GitHub

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

Jump to bottom

lib: enable dot-notation eslint rule #18007

Closed
wants to merge 1 commit into from
Closed

lib: enable dot-notation eslint rule #18007

wants to merge 1 commit into from

Conversation

apapirovski
Copy link
Member

There are currently only two instances of property access using square brackets (foo['bar']) within the lib folder. Fix those and enable an eslint rule to prefer dot notation (foo.bar).

Eventually this could be enabled across the project but currently this would cause too much churn (152 instances would need to be changed, mostly within test).

Refs: https://eslint.org/docs/rules/dot-notation

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

lib

@nodejs-github-bot nodejs-github-bot added inspector Issues and PRs related to the V8 inspector protocol module Issues and PRs related to the module subsystem. labels Jan 5, 2018
@apapirovski
Copy link
Member Author

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

@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. and removed inspector Issues and PRs related to the V8 inspector protocol module Issues and PRs related to the module subsystem. labels Jan 5, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 5, 2018
silverwind
silverwind approved these changes Jan 5, 2018
View reviewed changes
Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

LGTM, but I wouldn't mind if you do it for test too here.

@apapirovski
Copy link
Member Author

Landed in f3f3f88

@apapirovski apapirovski closed this Jan 9, 2018
@apapirovski apapirovski deleted the patch-eslint-dot-notation branch January 9, 2018 15:53
apapirovski added a commit that referenced this pull request Jan 9, 2018
@apapirovski
lib: enable dot-notation eslint rule
f3f3f88 
PR-URL: #18007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
@apapirovski @MylesBorins
lib: enable dot-notation eslint rule
5d7eb3d 
PR-URL: #18007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 10, 2018
@apapirovski @MylesBorins
lib: enable dot-notation eslint rule
b817a8a 
PR-URL: #18007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 24, 2018 •
edited
Loading

Should this land on LTS? it will need a manualy backport to both v8.x and v6.x

edit: got it to land on both release lines

MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@apapirovski @MylesBorins
lib: enable dot-notation eslint rule
92defcc 
PR-URL: #18007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@apapirovski @MylesBorins
lib: enable dot-notation eslint rule
969c39e 
PR-URL: #18007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
This was referenced Feb 27, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@eugeneo eugeneo eugeneo approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. 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.