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: enable ESLint recommended rule set #41463

Merged
merged 6 commits into from Jan 14, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 10, 2022

No description provided.

@Trott Trott added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jan 10, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. util Issues and PRs related to the built-in util module. labels Jan 10, 2022
@bnb
Copy link
Contributor

bnb commented Jan 10, 2022

It seems there's a mix of eslint-disable-next-line and eslint-disable-line. Would we want to standardize on one and roll with that?

@ljharb
Copy link
Member

ljharb commented Jan 10, 2022

Is there a reason not to permit both? I usually use whichever one is more readable, depending on the line length of the line i'm trying to disable the warning for.

@Trott
Copy link
Member Author

Trott commented Jan 11, 2022

It seems there's a mix of eslint-disable-next-line and eslint-disable-line. Would we want to standardize on one and roll with that?

I wouldn't want to standardize on one, but if we did, it would have to be eslint-disable-next-line because eslint-disable-line will sometimes result in a max-len violation.

@ljharb
Copy link
Member

ljharb commented Jan 11, 2022

(max-len should be disabled anyways; line length limits are a terrible way to manage complexity; but that’s a discussion not worth having here :-) )

@Trott
Copy link
Member Author

Trott commented Jan 11, 2022

(max-len should be disabled anyways; line length limits are a terrible way to manage complexity; but that’s a discussion not worth having here :-) )

I agree and have advocated for this in the past but met significant resistance. It might be time to bring it up again.

@bnb
Copy link
Contributor

bnb commented Jan 11, 2022

I agree and have advocated for this in the past but met significant resistance. It might be time to bring it up again.

I also agree and would be massively supportive.

@bnb
Copy link
Contributor

bnb commented Jan 11, 2022

I wouldn't want to standardize on one

no worries then. I figured it might be more readable (it is for my brain, since it requires less processing to match) but if we don't want to that's totally reasonable.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member

I agree and have advocated for this in the past but met significant resistance. It might be time to bring it up again.

I also agree and would be massively supportive.

+100. This is one of the biggest pains in getting code working, and it actually makes debugging harder. The current length is so short that lots of code is less readable because things like function calls and if conditionals need to be broken onto several lines.

@nodejs-github-bot
Copy link
Collaborator

Trott added a commit to Trott/io.js that referenced this pull request Jan 13, 2022
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 13, 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 13, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41463
✔  Done loading data for nodejs/node/pull/41463
----------------------------------- PR info ------------------------------------
Title      tools: enable ESLint recommended rule set (#41463)
Author     Rich Trott  (@Trott)
Branch     Trott:eslint-recommended-3 -> nodejs:master
Labels     util, tools, esm, needs-ci, commit-queue-rebase
Commits    6
 - tools: enable ESLint no-loss-of-precision rule
 - tools: enable ESLint no-sparse-arrays rule
 - tools: enable ESLint require-yield rule
 - tools,lib.test: enable ESLint no-regex-spaces rule
 - tools: enable ESLint no-constant-condition rule
 - tools: enable ESLint recommended configuration
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/41463
Reviewed-By: Tobias Nießen 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
Reviewed-By: Colin Ihrig 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41463
Reviewed-By: Tobias Nießen 
Reviewed-By: Michaël Zasso 
Reviewed-By: Luigi Pinca 
Reviewed-By: Colin Ihrig 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 10 Jan 2022 19:59:29 GMT
   ✔  Approvals: 5
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41463#pullrequestreview-848282703
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41463#pullrequestreview-848292420
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41463#pullrequestreview-848298916
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/41463#pullrequestreview-848305961
   ✔  - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/41463#pullrequestreview-852251982
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2022-01-13T22:20:05Z: https://ci.nodejs.org/job/node-test-pull-request/41860/
- Querying data for job/node-test-pull-request/41860/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1695349357

targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41463
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.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
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants