tools: enable no-empty ESLint rule by Trott · Pull Request #41831 · nodejs/node

Trott · 2022-02-03T06:37:20Z

Refs: https://eslint.org/docs/rules/no-empty

nodejs-github-bot · 2022-02-03T06:37:24Z

Review requested:

@nodejs/modules
@nodejs/startup

GeoffreyBooth · 2022-02-03T07:03:29Z

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

Trott · 2022-02-03T07:04:44Z

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

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.

benchmark/es/foreach-bench.js

benchmark/fs/bench-statSync-failure.js

test/node-api/test_fatal/test_threads.js

Trott · 2022-02-03T14:17:30Z

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 · 2022-02-03T15:14:28Z

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 · 2022-02-03T15:29:22Z

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());

doc/api/worker_threads.md

lib/events.js

Trott · 2022-02-04T02:02:56Z

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.

nodejs-github-bot · 2022-02-04T17:41:26Z

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

tniessen

LGTM with the changed loop style.

aduh95

LGTM modulo a few indentation nits.

test/pummel/test-policy-integrity-parent-module.js

test/pummel/test-policy-integrity-parent-no-package-json.js

test/pummel/test-policy-integrity-worker-commonjs.js

test/pummel/test-policy-integrity-worker-module.js

test/pummel/test-policy-integrity-worker-no-package-json.js

test/parallel/test-fs-realpath.js

test/parallel/test-file-write-stream2.js

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>

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>

Trott · 2022-03-01T03:47:52Z

@danielleadams Backport: #42160

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>

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>

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>

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>

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>

Refs: https://eslint.org/docs/rules/no-empty PR-URL: #41831 Backport-PR-URL: #42160 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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>

Trott 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 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-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2022

This comment was marked as outdated.

Sign in to view

Trott force-pushed the no-empty branch from 6d842a7 to 79581ac Compare February 3, 2022 07:03

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Feb 3, 2022

View reviewed changes

benchmark/es/foreach-bench.js Outdated Show resolved Hide resolved

benchmark/fs/bench-statSync-failure.js Outdated Show resolved Hide resolved

test/node-api/test_fatal/test_threads.js Outdated Show resolved Hide resolved

cjihrig approved these changes Feb 3, 2022

View reviewed changes

Trott force-pushed the no-empty branch from 79581ac to 8d46dbd Compare February 3, 2022 15:24

aduh95 reviewed Feb 3, 2022

View reviewed changes

doc/api/worker_threads.md Outdated Show resolved Hide resolved

aduh95 reviewed Feb 3, 2022

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

Trott force-pushed the no-empty branch from 8d46dbd to 329372c Compare February 3, 2022 18:36

Trott force-pushed the no-empty branch 2 times, most recently from 29a1717 to 2416923 Compare February 4, 2022 03:02

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022

tniessen approved these changes Feb 4, 2022

View reviewed changes

aduh95 approved these changes Feb 4, 2022

View reviewed changes

benchmark: enable no-empty ESLint rule

5c4a07b

Trott mentioned this pull request Mar 1, 2022

[16.x backport] tools: enable no-empty ESLint rule #42160

Closed

danielleadams added the backport-open-v16.x label Mar 1, 2022

danielleadams mentioned this pull request Mar 3, 2022

v16.14.1 release proposal #42200

Merged

Uh oh!

Conversation

Trott commented Feb 3, 2022

Uh oh!

nodejs-github-bot commented Feb 3, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

GeoffreyBooth commented Feb 3, 2022

Uh oh!

This comment was marked as outdated.

Trott commented Feb 3, 2022

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Feb 3, 2022

Uh oh!

aduh95 commented Feb 3, 2022

Uh oh!

tniessen commented Feb 3, 2022

Uh oh!

Uh oh!

Uh oh!

Trott commented Feb 4, 2022

Uh oh!

nodejs-github-bot commented Feb 4, 2022

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aduh95 left a comment •

edited

Loading