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

tools: enable ESLint recommended rule set by Trott · Pull Request #41463 · nodejs/node · GitHub

/ node Public

tools: enable ESLint recommended rule set - #41463

Merged
Trott merged 6 commits into
nodejs:masterfrom
Trott:eslint-recommended-3
Jan 14, 2022
Merged

tools: enable ESLint recommended rule set#41463
Trott merged 6 commits into
nodejs:masterfrom
Trott:eslint-recommended-3

Conversation

Trott commented Jan 10, 2022

Copy link
Copy Markdown
Member

No description provided.

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2022

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
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>
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 mentioned this pull request Feb 1, 2022
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
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.

9 participants


Back | FazBrowse Home | New Git URL