Skip to content

lib: enable dot-notation eslint rule#18007

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
Closed
apapirovski wants to merge 1 commit into from
Closed

lib: enable dot-notation eslint rule #18007

apapirovski wants to merge 1 commit into from

Conversation

@apapirovski
Copy link
Contributor

@apapirovskiapapirovski commented Jan 5, 2018

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-botnodejs-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
ContributorAuthor

apapirovski commented Jan 5, 2018

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

@addaleaxaddaleax 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
@BridgeARBridgeAR 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

@silverwindsilverwind 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
ContributorAuthor

apapirovski commented Jan 9, 2018

Landed in f3f3f88

@apapirovskiapapirovski 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]>
@MylesBorinsMylesBorins 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]>
@TimothyGuTimothyGu 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@TrottTrottTrott approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@lpincalpincalpinca approved these changes

@cjihrigcjihrigcjihrig approved these changes

@BridgeARBridgeARBridgeAR approved these changes

+3 more reviewers

@silverwindsilverwindsilverwind approved these changes

@eugeneoeugeneoeugeneo approved these changes

@maclover7maclover7maclover7 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@apapirovski@MylesBorins@silverwind@eugeneo@jasnell@Trott@addaleax@lpinca@cjihrig@maclover7@BridgeAR@TimothyGu@nodejs-github-bot