Skip to content

tools: enable no-empty ESLint rule - #41831

Closed
Trott wants to merge 8 commits into
nodejs:masterfrom
Trott:no-empty
Closed

tools: enable no-empty ESLint rule#41831
Trott wants to merge 8 commits into
nodejs:masterfrom
Trott:no-empty

Conversation

@Trott

@TrottTrott commented Feb 3, 2022

Copy link
Copy Markdown
Member

@TrottTrott added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Feb 3, 2022
@nodejs-github-bot

nodejs-github-bot commented Feb 3, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-botnodejs-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 Feb 3, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@GeoffreyBooth

GeoffreyBooth commented Feb 3, 2022

Copy link
Copy Markdown
Member

What's the motivation for this change? It seems fine; is the goal to gradually align with the recommended rules?

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Trott

Trott commented Feb 3, 2022

Copy link
Copy Markdown
MemberAuthor

What's the motivation for this change? It seems fine; is the goal to gradually align with the recommended rules?

Yes. We have only two rules from the recommended set disabled, and they both seem like things we minimally-can-and-likely-should accommodate.

aduh95
aduh95 reviewed Feb 3, 2022
View reviewed changes

@aduh95aduh95 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this rule, there are a lot of examples in this PR that shows how empty blocks are sometimes useful. To be clear I'm not blocking.

Comment threadbenchmark/es/foreach-bench.js Outdated
Comment threadbenchmark/fs/bench-statSync-failure.js Outdated
Comment threadtest/node-api/test_fatal/test_threads.js Outdated
@Trott

Trott commented Feb 3, 2022

Copy link
Copy Markdown
MemberAuthor

I'm not sure about this rule, there are a lot of examples in this PR that shows how empty blocks are sometimes useful. To be clear I'm not blocking.

Depending how many examples there are, maybe a disable comment for the useful ones would be good?

@aduh95

aduh95 commented Feb 3, 2022

Copy link
Copy Markdown
Contributor

I'm not sure about this rule, there are a lot of examples in this PR that shows how empty blocks are sometimes useful. To be clear I'm not blocking.

Depending how many examples there are, maybe a disable comment for the useful ones would be good?

Sure that sounds good. Let me know if you need help, I can contribute.

@tniessen

tniessen commented Feb 3, 2022

Copy link
Copy Markdown
Member

Is there a reason to keep empty loop blocks at all? This PR adds a lot of blocks of the form

while(foo()){// empty}

I think what I'd prefer is:

while(foo());

aduh95
aduh95 reviewed Feb 3, 2022
View reviewed changes
Comment threaddoc/api/worker_threads.md Outdated
aduh95
aduh95 reviewed Feb 3, 2022
View reviewed changes
Comment threadlib/events.js Outdated
@Trott

Trott commented Feb 4, 2022

Copy link
Copy Markdown
MemberAuthor

Is there a reason to keep empty loop blocks at all? This PR adds a lot of blocks of the form

while(foo()){// empty}

I think what I'd prefer is:

while(foo());

Whoa, yes, that seems to work.

@Trott
Trottforce-pushed the no-empty branch 2 times, most recently from 29a1717 to 2416923CompareFebruary 4, 2022 03:02
@TrottTrott added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@nodejs-github-bot

nodejs-github-bot commented Feb 4, 2022

Copy link
Copy Markdown
Collaborator

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

tniessen
tniessen approved these changes Feb 4, 2022
View reviewed changes

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with the changed loop style.

aduh95
aduh95 approved these changes Feb 4, 2022
View reviewed changes

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM modulo a few indentation nits.

Comment threadtest/pummel/test-policy-integrity-parent-module.js Outdated
Comment threadtest/pummel/test-policy-integrity-parent-no-package-json.js Outdated
Comment threadtest/pummel/test-policy-integrity-worker-commonjs.js Outdated
Comment threadtest/pummel/test-policy-integrity-worker-module.js Outdated
Comment threadtest/pummel/test-policy-integrity-worker-no-package-json.js Outdated
Comment threadtest/parallel/test-fs-realpath.js Outdated
Comment threadtest/parallel/test-fs-realpath.js Outdated
Comment threadtest/parallel/test-fs-realpath.js Outdated
Comment threadtest/parallel/test-fs-realpath.js Outdated
Comment threadtest/parallel/test-file-write-stream2.js Outdated
@TrottTrott mentioned this pull request Mar 1, 2022
Closed
@Trott

Trott commented Mar 1, 2022

Copy link
Copy Markdown
MemberAuthor

@danielleadams Backport: #42160

Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
benchmark: enable no-empty ESLint rule
bf3bb92
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
doc: remove empty block from console.timeEnd() example
a24b61a
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
doc: add comments to empty blocks in worker_threads text
7c0cd14
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
lib: add comments to empty catch statements
0038697
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
policy: revise manifest.js to avoid empty blocks
b35692f
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
stream: remove empty block
d00c97a
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
test: enable no-empty ESLint rule
48f5a12
PR-URL: nodejs#41831
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2022
@Trott
tools: enable no-empty ESLint rule
01c3818
Refs: https://eslint.org/docs/rules/no-empty
PR-URL: nodejs#41831
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott@danielleadams
benchmark: enable no-empty ESLint rule
150adf7
PR-URL: #41831
Backport-PR-URL: #42160
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott@danielleadams
doc: remove empty block from console.timeEnd() example
dd3d6c9
PR-URL: #41831
Backport-PR-URL: #42160
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott@danielleadams
doc: add comments to empty blocks in worker_threads text
fcfb023
PR-URL: #41831
Backport-PR-URL: #42160
Refs: https://eslint.org/docs/rules/no-empty
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
@tniessentniessentniessen approved these changes
@aduh95aduh95aduh95 approved these changes

Assignees

No one assigned

Labels

commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Trott@nodejs-github-bot@GeoffreyBooth@aduh95@tniessen@danielleadams@cjihrig@targos