GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: remove conditional assignment in custom ESLint rule by Trott · Pull Request #41325 · nodejs/node

Conversation

@Trott

@Trott 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nschonni reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 26, 2021
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 26, 2021

Copy link
Copy Markdown
Member Author

@nodejs/linting
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the no-cond-assign branch from 555b6f5 to 596c45b Compare December 26, 2021 05:08
@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

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos approved these changes Jan 3, 2022
View reviewed changes
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Jan 3, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 3, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Jan 4, 2022
CI Reliability 2022-01-04 nodejs/reliability#165
Open
25 tasks
@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Jan 5, 2022
CI Reliability 2022-01-05 nodejs/reliability#166
Open
26 tasks
@nodejs-github-bot

nodejs-github-bot commented Jan 5, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 5, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 5, 2022
@nodejs-github-bot

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Jan 5, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 5, 2022
@nodejs-github-bot

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
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 <[email protected]>
@Trott
Trott force-pushed the no-cond-assign branch from 596c45b to e7d4e6b Compare January 5, 2022 14:11
@Trott

Trott commented Jan 5, 2022

Copy link
Copy Markdown
Member Author

Landed in e7d4e6b
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott merged commit e7d4e6b into nodejs:master Jan 5, 2022
@Trott
Trott deleted the no-cond-assign branch January 5, 2022 14:12
This was referenced Jan 6, 2022
CI Reliability 2022-01-06 nodejs/reliability#167
Open
CI Reliability 2022-01-07 nodejs/reliability#168
Open
CI Reliability 2022-01-08 nodejs/reliability#169
Open
CI Reliability 2022-01-09 nodejs/reliability#170
Open
CI Reliability 2022-01-12 nodejs/reliability#171
Open
targos pushed a commit that referenced this pull request Jan 14, 2022
@Trott @targos
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 <[email protected]>
@targos targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@Trott @danielleadams
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 <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
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 <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
v16.14.0 proposal #41804
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. 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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@Trott @targos @nodejs-github-bot @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.