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: remove conditional assignment in custom ESLint rule #41325

Merged
merged 1 commit into from Jan 5, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 26, 2021

These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 26, 2021
@Trott
Copy link
Member Author

Trott commented Dec 26, 2021

This one is a bit cosmetic, so I should explain my eventual goal: I'd like to be able to enable ESLint's recommended rule set (disabling a few rules as necessary) and this file is flagged for the conditional assignment. I don't feel strongly that this version is a big improvement so if others don't see it that way, oh well, we can close it. But I don't think it makes the code worse either.

@Trott
Copy link
Member Author

Trott commented Dec 26, 2021

@nodejs/linting

@Trott Trott changed the title tools remove conditional assignment in custom ESLint rule tools: remove conditional assignment in custom ESLint rule Dec 26, 2021
@targos
Copy link
Member

targos commented Dec 26, 2021

I think it's possible to silence the eslint error by wrapping the assignment in parentheses

@Trott
Copy link
Member Author

Trott commented Dec 26, 2021

I think it's possible to silence the eslint error by wrapping the assignment in parentheses

Yes, that is correct. I considered that, but while ((node = node.parent)) is harder for me to understand/read. It raises the questions when I read the code: "Why is there an extra set of parentheses?" It's not immediately obvious, at least to me. But while ((node = node.parent) === undefined)) is perhaps a bit better.

@Trott
Copy link
Member Author

Trott commented Dec 31, 2021

I think it's possible to silence the eslint error by wrapping the assignment in parentheses

@targos Do you have opinions as to which options are preferable and which should be avoided?

  1. Do nothing. Don't enable the lint rule.
  2. Enable the rule (eventually), but disable it in this code with an eslint-disable-line (or similar) comment so it's clear that the assignment is intentional.
  3. Use double parentheses to show that it's intentional. while ((node = node.parent))
  4. Use double parentheses with an explicit comparison to show that it's intentional. while ((node = node.parent) === undefined))
  5. Rewrite the code to avoid the assignment entirely, as in this PR.

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41325
✔  Done loading data for nodejs/node/pull/41325
----------------------------------- PR info ------------------------------------
Title      tools: remove conditional assignment in custom ESLint rule (#41325)
Author     Rich Trott  (@Trott)
Branch     Trott:no-cond-assign -> nodejs:master
Labels     tools, author ready
Commits    1
 - tools: remove conditional assignment in custom ESLint rule
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/41325
Reviewed-By: Michaël Zasso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41325
Reviewed-By: Michaël Zasso 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 26 Dec 2021 03:55:17 GMT
   ✔  Approvals: 1
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41325#pullrequestreview-842514715
   ✖  GitHub CI is still running
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1657795258

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 5, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41325
✔  Done loading data for nodejs/node/pull/41325
----------------------------------- PR info ------------------------------------
Title      tools: remove conditional assignment in custom ESLint rule (#41325)
Author     Rich Trott  (@Trott)
Branch     Trott:no-cond-assign -> nodejs:master
Labels     tools, author ready
Commits    1
 - tools: remove conditional assignment in custom ESLint rule
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/41325
Reviewed-By: Michaël Zasso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41325
Reviewed-By: Michaël Zasso 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 26 Dec 2021 03:55:17 GMT
   ✔  Approvals: 1
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41325#pullrequestreview-842514715
   ✖  GitHub CI is still running
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1658007831

These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

PR-URL: nodejs#41325
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott
Copy link
Member Author

Trott commented Jan 5, 2022

Landed in e7d4e6b

@Trott Trott merged commit e7d4e6b into nodejs:master Jan 5, 2022
@Trott Trott deleted the no-cond-assign branch January 5, 2022 14:12
targos pushed a commit that referenced this pull request Jan 14, 2022
These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

PR-URL: #41325
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

PR-URL: #41325
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

PR-URL: nodejs#41325
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
These changes no-duplicate-require.js so that it doesn't use an
assignment in a conditional, which can be easy to misread as a
comparison rather than an assignment. It also means we change a do/while
(which we don't use much in our code) to the much more common while
construct.

PR-URL: #41325
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants