GeezerVison

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

GeezerVison is showing github.com — tools,benchmark,lib,test: enable no-case-declarations lint rule by Trott · Pull Request #41385 · nodejs/node

Conversation

Trott commented Jan 3, 2022

Copy link

Copy Markdown

Member

Don't leak identifiers into other case declarations. This is an ESLint recommended rule. My goal/hope is to be able to enable eslint:recommended at some point and have far fewer individual rules specified in .eslintrc.js.

Sorry, something went wrong.

Uh oh!

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

All reactions

nodejs-github-bot commented Jan 3, 2022

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/crypto
@nodejs/http
@nodejs/http2
@nodejs/net
@nodejs/startup

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot added lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci PRs that need a full CI run. labels

Jan 3, 2022

Trott added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci PRs that need a full CI run. labels

Jan 3, 2022

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

This comment has been minimized.

tniessen commented Jan 3, 2022

Copy link

Copy Markdown

Member

Do you know if there is (or should be) a predefined rule that enforces that either all or no cases belonging to a switch statement are wrapped in code blocks? I don't know if that would be a good idea, these braces for the sake of scopes in case have just always been bothering me :)

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Jan 3, 2022

Copy link

Copy Markdown

Member Author

Do you know if there is (or should be) a predefined rule that enforces that either all or no cases belonging to a switch statement are wrapped in code blocks?

I'm unaware of anything like that in ESLint core. There might be a third-party module available, and it shouldn't be too hard to write one ourselves. (I'd be -0 on doing that because it's extra maintenance for IMO little benefit. But if you or someone else feels strongly about it, I wouldn't try to persuade you to not do it.)

All reactions

Sorry, something went wrong.

Uh oh!

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

This comment has been minimized.

targos approved these changes

Jan 3, 2022

View reviewed changes

aduh95 approved these changes

Jan 3, 2022

View reviewed changes

tniessen approved these changes

Jan 3, 2022

View reviewed changes

tniessen left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure how I feel about the additional inconsistencies between cases (aesthetically).

Sorry, something went wrong.

Uh oh!

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

All reactions

nodejs-github-bot commented Jan 3, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bnb approved these changes

Jan 3, 2022

View reviewed changes

bnb left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Also agree with the eventual goal of eslint recommended and happy to see work to get us closer to that :)

Sorry, something went wrong.

Uh oh!

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

All reactions

github-actions Bot mentioned this pull request

Jan 4, 2022

CI Reliability 2022-01-04 nodejs/reliability#165

Open

25 tasks

rickyes approved these changes

Jan 4, 2022

View reviewed changes

cjihrig approved these changes

Jan 4, 2022

View reviewed changes

nodejs-github-bot commented Jan 4, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot mentioned this pull request

Jan 5, 2022

CI Reliability 2022-01-05 nodejs/reliability#166

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/41752/

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Jan 5, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott added the commit-queue PRs queued for automated landing through the Commit Queue. label

Jan 5, 2022

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

Copy link

Copy Markdown

Collaborator

Commit Queue failed

- Loading data for nodejs/node/pull/41385
✔  Done loading data for nodejs/node/pull/41385
----------------------------------- PR info ------------------------------------
Title      tools,benchmark,lib,test: enable no-case-declarations lint rule (#41385)
Author     Rich Trott  (@Trott)
Branch     Trott:no-case-declaration -> nodejs:master
Labels     
Commits    1
 - tools,benchmark,lib,test: enable no-case-declarations lint rule
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/41385
Reviewed-By: Michaël Zasso 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Tobias Nießen 
Reviewed-By: Tierney Cyren 
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41385
Reviewed-By: Michaël Zasso 
Reviewed-By: Antoine du Hamel 
Reviewed-By: Tobias Nießen 
Reviewed-By: Tierney Cyren 
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 03 Jan 2022 04:26:25 GMT
   ✔  Approvals: 6
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41385#pullrequestreview-842509110
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/41385#pullrequestreview-842568613
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41385#pullrequestreview-842952082
   ✔  - Tierney Cyren (@bnb): https://github.com/nodejs/node/pull/41385#pullrequestreview-842990323
   ✔  - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/41385#pullrequestreview-843192744
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/41385#pullrequestreview-843588295
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2022-01-05T14:09:03Z: https://ci.nodejs.org/job/node-test-pull-request/41763/
- Querying data for job/node-test-pull-request/41763/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1659016743

All reactions

Sorry, something went wrong.

Uh oh!

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


          tools,benchmark,lib,test: enable no-case-declarations lint rule

55ceaec

PR-URL: nodejs#41385
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Trott force-pushed the no-case-declaration branch from 2115a8e to 55ceaec Compare

January 5, 2022 15:42

Trott commented Jan 5, 2022

Copy link

Copy Markdown

Member Author

Landed in 55ceaec

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott merged commit 55ceaec into nodejs:master

Jan 5, 2022

Trott deleted the no-case-declaration branch

January 5, 2022 15:43

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


          tools,benchmark,lib,test: enable no-case-declarations lint rule

800640a

PR-URL: #41385
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

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


          tools,benchmark,lib,test: enable no-case-declarations lint rule

22083a0

PR-URL: #41385
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

danielleadams pushed a commit that referenced this pull request

Feb 1, 2022


          tools,benchmark,lib,test: enable no-case-declarations lint rule

362f5ed

PR-URL: #41385
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

danielleadams mentioned this pull request

Feb 1, 2022

v16.14.0 proposal #41804

Merged

Reviewers

bnb

bnb approved these changes

cjihrig

cjihrig approved these changes

tniessen

tniessen approved these changes

aduh95

aduh95 approved these changes

+2 more reviewers

targos

targos approved these changes

rickyes

rickyes 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

commit-queue-failed

PRs whose Commit Queue landing failed and need manual intervention before retrying.

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.

8 participants

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.