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

tools: enable no-self-assign ESLint rule #5552

Closed
wants to merge 2 commits into from
Closed

tools: enable no-self-assign ESLint rule #5552

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 3, 2016

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Mar 3, 2016
@jasnell
Copy link
Member

jasnell commented Mar 3, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Mar 3, 2016

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

@cjihrig
Copy link
Contributor

cjihrig commented Mar 3, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Mar 4, 2016

CI failure looks infra related and unrelated to this change.

Trying again: https://ci.nodejs.org/job/node-test-pull-request/1844/

@jbergstroem
Copy link
Member

LGTM

@silverwind
Copy link
Contributor

Updated the link for Strict rules to point to the ESLint website rather than the GitHub-hosted code

There's two more links above pointing to Github, can you change them too while you're here?

@Trott
tools: enable no-self-assign ESLint rule
d03d0db 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.
@Trott
fixup per silverwind
2ebe5f3
@Trott
Copy link
Member Author

Trott commented Mar 5, 2016

@silverwind Sure, done.

@silverwind
Copy link
Contributor

LGTM

@ronkorving
Copy link
Contributor

Why the change of buffer-base64-decode.js? Seems a better fit for #5359.

@Trott
Copy link
Member Author

Trott commented Mar 7, 2016

@ronkorving asked:

Why the change of buffer-base64-decode.js?

I changed it because (due to s += s) it's the only file in the entire code base that fails this lint rule. Switching to String.prototype.repeat() seemed like the obvious thing to do.

@ronkorving
Copy link
Contributor

Oh wow. Alright 👍

@targos
Copy link
Member

targos commented Mar 7, 2016

LGTM

@targos
Copy link
Member

targos commented Mar 7, 2016

IMO the warning in buffer-base64-decode.js is due to an ESLint bug. There was no self-assignment there.

@silverwind
Copy link
Contributor

@targos you're right. The rule defines self-assignment as something that has no effect, but in this case it obviously has. Perf on the two methods is basically the same, so it doesn't matter much here though.

@Trott
Copy link
Member Author

Trott commented Mar 7, 2016

@targos @silverwind I agree, but in this particular case, the line being flagged is better as rewritten, so I'm OK with it. We are a couple versions behind on ESLint now. I wonder if a more recent release wouldn't flag it.

@targos
Copy link
Member

targos commented Mar 7, 2016

I do not question your changes (I LGTM'd them after all). The file is indeed better rewritten. I was just mentioning it in case someone wants to file an issue in the ESLint repo.

Trott added a commit to Trott/io.js that referenced this pull request Mar 7, 2016
@Trott
tools: enable no-self-assign ESLint rule
810aa9f 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: nodejs#5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Trott
Copy link
Member Author

Trott commented Mar 7, 2016

Landed in 810aa9f

@Trott Trott closed this Mar 7, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2016

It looks like this has been fixed in ESLint, and just hasn't been released yet - eslint/eslint#5371.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2016

Sorry, the fix went out on March 4th in [email protected].

@Fishrock123 Fishrock123 mentioned this pull request Mar 7, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@Trott @Fishrock123
tools: enable no-self-assign ESLint rule
3e41c13 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: #5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@Trott @Fishrock123
tools: enable no-self-assign ESLint rule
7965c89 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: #5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
@Trott
tools: enable no-self-assign ESLint rule
8c94cc2 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: #5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@Trott
tools: enable no-self-assign ESLint rule
1f5f341 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: #5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@Trott
tools: enable no-self-assign ESLint rule
64d5752 
Enabled no-self-assign rule in ESLint.

This required one change in a benchmark file. Changed a loop (that is
outside of the benchmark itself, so performance is not critical) from a
for loop that repeats a string to use String.prototype.repeat() instead.

While at it, took the opportunity to const-ify the benchmark file.

Also moved the "Strict" section in the .eslintrc to match where it is in
the ESLint documentation. Updated the link for Strict rules to point to
the ESLint website rather than the GitHub-hosted code.

PR-URL: #5552
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Trott Trott deleted the lint-lint-lint branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. 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.
8 participants
@Trott @jasnell @cjihrig @jbergstroem @silverwind @ronkorving @targos @MylesBorins