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,benchmark,lib,test: enable no-case-declarations lint rule #41385

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 3, 2022

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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 3, 2022
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. 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.

@tniessen
Copy link
Member

tniessen commented Jan 3, 2022

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 :)

@Trott
Copy link
Member Author

Trott commented Jan 3, 2022

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.)

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@tniessen tniessen left a 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).

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@bnb bnb left a 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 :)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

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
Copy link
Member Author

Trott commented Jan 5, 2022

Landed in 55ceaec

@Trott Trott merged commit 55ceaec into nodejs:master Jan 5, 2022
@Trott Trott deleted the no-case-declaration branch January 5, 2022 15:43
targos pushed a commit that referenced this pull request Jan 14, 2022
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 Jan 31, 2022
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]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
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 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
commit-queue-failed An error occurred while landing this pull request using GitHub Actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants