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

Fixed typo. Removed useless "?". #42518

Closed
wants to merge 1 commit into from
Closed

Fixed typo. Removed useless "?". #42518

wants to merge 1 commit into from

Conversation

alsoamit
Copy link

No description provided.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 30, 2022
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

The ? here is not useless as it is ensuring buffer is not null or undefined before checking the value of .byteLength. It is part of the optional chaining operator.

@aduh95
Copy link
Contributor

aduh95 commented Mar 30, 2022

The ? here is not useless as it is ensuring buffer is not null or undefined before checking the value of .byteLength. It is part of the optional chaining operator.

Looks like tests are passing with this change, so maybe we don't need the optional chaining here? Maybe @alsoamit you meant "unused" rather than "useless"?

@mscdex
Copy link
Contributor

mscdex commented Mar 30, 2022

Looks like tests are passing with this change

AFAICT this is a user-facing function, so maybe the tests are lacking?

@aduh95
Copy link
Contributor

aduh95 commented Mar 30, 2022

Looks like tests are passing with this change

AFAICT this is a user-facing function, so maybe the tests are lacking?

It's not a user-facing function, there's no fs.promises.write function, it's only used internally AFAIK. EDIT: although there's FileHandle.prototype.write that uses it, without validating its arguments it looks like. We should make sure we have tests for that.

@alsoamit alsoamit closed this Mar 30, 2022
aduh95 added a commit to aduh95/node that referenced this pull request Mar 31, 2022
nodejs-github-bot pushed a commit that referenced this pull request Apr 2, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Apr 3, 2022
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Apr 3, 2022
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
Refs: nodejs#42518

PR-URL: nodejs#42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Refs: nodejs#42518

PR-URL: nodejs#42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
Refs: #42518

PR-URL: #42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Refs: nodejs/node#42518

PR-URL: nodejs/node#42541
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants