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.

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

lib: prefer logical assignment #55044

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Sep 21, 2024

Enforces https://eslint.org/docs/latest/rules/logical-assignment-operators


IMO logical assignment operators are more concise and readable than if/then and || statements. This way, we can do more in less code, while keeping the files readable.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 21, 2024
@RedYetiDev RedYetiDev added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 21, 2024
Copy link

codecov bot commented Sep 21, 2024

Codecov Report

Attention: Patch coverage is 97.08738% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.71%. Comparing base (aac8ba7) to head (c46eb6d).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/_http_server.js 75.00% 1 Missing ⚠️
lib/internal/child_process.js 50.00% 0 Missing and 1 partial ⚠️
lib/internal/test_runner/utils.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55044      +/-   ##
==========================================
- Coverage   88.24%   87.71%   -0.54%     
==========================================
  Files         651      651              
  Lines      183856   183775      -81     
  Branches    35853    35462     -391     
==========================================
- Hits       162251   161198    -1053     
- Misses      14896    15824     +928     
- Partials     6709     6753      +44     
Files with missing lines Coverage Δ
lib/_http_agent.js 98.17% <100.00%> (-0.02%) ⬇️
lib/_http_client.js 97.96% <100.00%> (ø)
lib/_tls_common.js 97.43% <100.00%> (-0.02%) ⬇️
lib/_tls_wrap.js 93.38% <100.00%> (ø)
lib/assert.js 99.87% <100.00%> (ø)
lib/async_hooks.js 99.65% <100.00%> (ø)
lib/child_process.js 97.43% <100.00%> (-0.31%) ⬇️
lib/dgram.js 97.31% <100.00%> (-0.01%) ⬇️
lib/events.js 98.18% <100.00%> (-1.66%) ⬇️
lib/internal/assert.js 100.00% <100.00%> (ø)
... and 32 more

... and 79 files with indirect coverage changes

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

This is a cosmetic change, and needs to include an eslint rule in order to land, or someone can easily change it in the future. cc @lpinca

@RedYetiDev RedYetiDev marked this pull request as draft September 21, 2024 23:18
@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Sep 21, 2024
@RedYetiDev
Copy link
Member Author

WIP until I develop a lint rule.

@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Sep 22, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review September 22, 2024 00:21
@RedYetiDev RedYetiDev changed the title lib: simplify options lazy-loading lib: prefer logical assignment Sep 22, 2024
@RedYetiDev
Copy link
Member Author

@anonrig I enforced the logical-assignment-operators lint rule. Are you still blocking?

lib/_http_client.js Outdated Show resolved Hide resolved
lib/internal/bootstrap/switches/is_not_main_thread.js Outdated Show resolved Hide resolved
lib/internal/console/constructor.js Outdated Show resolved Hide resolved
lib/internal/console/constructor.js Outdated Show resolved Hide resolved
lib/internal/fs/utils.js Outdated Show resolved Hide resolved
lib/internal/fs/utils.js Outdated Show resolved Hide resolved
lib/internal/modules/helpers.js Outdated Show resolved Hide resolved
tools/doc/json.mjs Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev force-pushed the fixup-options branch 2 times, most recently from a46a373 to e795fd7 Compare September 22, 2024 21:54
@anonrig
Copy link
Member

anonrig commented Sep 22, 2024

74 files is almost impossible to review for me.

lib/_tls_common.js Outdated Show resolved Hide resolved
lib/async_hooks.js Outdated Show resolved Hide resolved
lib/internal/fs/sync_write_stream.js Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Sep 22, 2024

@jasnell @trivikr @atlowChemi please rereview since the changes you approved are no longer the same.

@anonrig
Copy link
Member

anonrig commented Sep 22, 2024

@RedYetiDev what is the motivation for this change, please add it to your description for reviewers

@RedYetiDev
Copy link
Member Author

@anonrig sorry for the little mistakes in this PR, i hope it's all good now. Thank you so much for your patience (everyone really)

@RedYetiDev
Copy link
Member Author

Okay. This should be A-OK now, could everyone have another look?

lib/events.js Outdated Show resolved Hide resolved
lib/internal/options.js Outdated Show resolved Hide resolved
lib/internal/streams/duplex.js Outdated Show resolved Hide resolved
lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved
lib/readline.js Outdated Show resolved Hide resolved
lib/timers.js Outdated Show resolved Hide resolved
lib/timers.js Outdated Show resolved Hide resolved
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

LGTM.
For future PRs of this size, I'd recommend to commit fixups separately (i.e. even if rebase is required, it's better to push fixup first and then force-push it rebased on main, so it's easier to review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants