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.

Sign up for GitHub

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

Jump to bottom

tools: enable no-unused-expressions lint rule #36248

Closed
wants to merge 1 commit into from
Closed

tools: enable no-unused-expressions lint rule #36248

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Nov 24, 2020 •
edited
Loading

Fixes: #36246

There are many places where I needed to disable the rule but I think it's still worth to have it. A few real mistakes are fixed thanks to it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 24, 2020
aduh95
aduh95 approved these changes Nov 24, 2020
View reviewed changes
lib/internal/errors.js Show resolved Hide resolved
kaizhu256
kaizhu256 reviewed Nov 25, 2020
View reviewed changes
benchmark/perf_hooks/bench-eventlooputil.js
@@ -33,7 +33,7 @@ function main({ method, n }) {
function benchIdleTime(n) {
bench.start();
for (let i = 0; i < n; i++)
nodeTiming.idleTime;
nodeTiming.idleTime; // eslint-disable-line no-unused-expressions
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be wrapped inside a noop or would it significantly affect the benchmark?

+function noop() {
+    return;
+}
...
-    nodeTiming.idleTime; // eslint-disable-line no-unused-expressions
+    noop(nodeTiming.idleTime);

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to explicitly disable a rule when necessary instead of working around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok i agree - gives more eyeballs to scrutinize these weird pieces of code.
maybe adding a comment justifying naked-property-access could help catch more bugs in the pr (like whether they're actually needed or not)?

+    // benchmark getter() idleTime
     nodeTiming.idleTime; // eslint-disable-line no-unused-expressions

@targos targos force-pushed the no-unused-expressions branch from a1a77c1 to 2a0e98e Compare December 6, 2020 15:34
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

targos added a commit that referenced this pull request Dec 7, 2020
@targos
tools: enable no-unused-expressions lint rule
bf31d3c 
Fixes: #36246

PR-URL: #36248
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member Author

targos commented Dec 7, 2020

Landed in bf31d3c

@targos targos closed this Dec 7, 2020
@targos targos deleted the no-unused-expressions branch December 7, 2020 19:37
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
@targos @cjihrig
tools: enable no-unused-expressions lint rule
6e3da3b 
Fixes: nodejs#36246

PR-URL: nodejs#36248
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Dec 21, 2020
@targos
tools: enable no-unused-expressions lint rule
040b39f 
Fixes: #36246

PR-URL: #36248
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
Merged
targos added a commit that referenced this pull request May 16, 2021
@targos
tools: enable no-unused-expressions lint rule
b39e71b 
Fixes: #36246

PR-URL: #36248
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos added a commit that referenced this pull request Jun 11, 2021
@targos
tools: enable no-unused-expressions lint rule
226a86c 
Fixes: #36246

PR-URL: #36248
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaizhu256 kaizhu256 kaizhu256 left review comments

@whiteslack whiteslack whiteslack approved these changes

@ZYSzys ZYSzys ZYSzys approved these changes

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

There is no linter warning for an async function that is not called