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 Backport-PR-URL: nodejs#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: nodejs#41831 Backport-PR-URL: nodejs#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>

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>

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>

nodejs/node#41831

* chore: bump node in DEPS to v16.14.0 * src: add flags for controlling process behavior nodejs/node#40339 * src: add x509.fingerprint512 to crypto module nodejs/node#39809 * deps: upgrade to libuv 1.43.0 nodejs/node#41398 * chore: fixup patch indices * chore: add missing filenames nodejs/node#39283 nodejs/node#40665 * crypto: trim input for NETSCAPE_SPKI_b64_decode nodejs/node#40757 * chore: update patches * chore: bump node in DEPS to v16.14.1 * tools: enable no-empty ESLint rule nodejs/node#41831 * chore: update patches * chore: update patches * chore: bump node in DEPS to v16.14.2 * chore: update patches Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.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

codebytere added a commit to electron/electron that referenced this pull request Mar 16, 2022

tools: enable no-empty ESLint rule

e6107c8

nodejs/node#41831

codebytere added a commit to electron/electron that referenced this pull request Mar 17, 2022

tools: enable no-empty ESLint rule

9bc11d9

nodejs/node#41831

targos removed backport-requested-v16.x labels Jul 31, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aduh95 left a comment •

edited

Loading