FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: remove conditional assignment in custom ESLint rule by Trott · Pull Request #41325 · nodejs/node · GitHub

/ node Public

tools: remove conditional assignment in custom ESLint rule - #41325

Merged
Trott merged 1 commit into
nodejs:masterfrom
Trott:no-cond-assign
Jan 5, 2022
ConversationCommits (1)ChecksFiles changed
Merged

tools: remove conditional assignment in custom ESLint rule#41325
Trott merged 1 commit into
nodejs:masterfrom
Trott:no-cond-assign

Conversation

Trott commented Dec 26, 2021

Copy link
Copy Markdown
Member

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 added the tools Issues and PRs related to the tools directory. label Dec 26, 2021

Trott commented Dec 26, 2021

Copy link
Copy Markdown
Member Author

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 commented Dec 26, 2021

Copy link
Copy Markdown
Member Author

@nodejs/linting

Trott changed the title tools remove conditional assignment in custom ESLint rule tools: remove conditional assignment in custom ESLint rule Dec 26, 2021

targos commented Dec 26, 2021

Copy link
Copy Markdown
Member

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

Trott commented Dec 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Dec 31, 2021

Copy link
Copy Markdown
Member Author

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 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jan 3, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2022

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
github-actions Bot mentioned this pull request Jan 4, 2022
Open
25 tasks

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Jan 5, 2022
Open
26 tasks

nodejs-github-bot commented Jan 5, 2022

Copy link
Copy Markdown
Collaborator

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2022
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 commented Jan 5, 2022

Copy link
Copy Markdown
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 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 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 commented Jan 5, 2022

Copy link
Copy Markdown
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

tools: remove conditional assignment in custom ESLint rule
e7d4e6b 
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 commented Jan 5, 2022

Copy link
Copy Markdown
Member Author

Landed in e7d4e6b

Trott merged commit e7d4e6b into nodejs:master Jan 5, 2022
Trott deleted the no-cond-assign branch January 5, 2022 14:12
This was referenced Jan 6, 2022
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Jan 14, 2022
tools: remove conditional assignment in custom ESLint rule
dcada80 
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>
targos mentioned this pull request Jan 16, 2022
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
tools: remove conditional assignment in custom ESLint rule
4110b3f 
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 Feb 1, 2022
tools: remove conditional assignment in custom ESLint rule
3ecc964 
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 mentioned this pull request Feb 1, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL