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

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
mscdex
mscdex suggested changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@mscdex mscdex left a comment •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
@aduh95
test: improve FileHandle.prototype.write coverage
e588592 
Refs: nodejs#42518
@aduh95 aduh95 mentioned this pull request Mar 31, 2022
Merged
nodejs-github-bot pushed a commit that referenced this pull request Apr 2, 2022
@aduh95
test: improve FileHandle.prototype.write coverage
46f54bd 
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
squash: remove unused optional chaining
8d901be 
Refs: nodejs#42518
@LiviaMedeiros LiviaMedeiros mentioned this pull request Apr 3, 2022
Merged
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Apr 3, 2022
@LiviaMedeiros
squash: remove unused optional chaining
ac6f1df 
Refs: nodejs#42518
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@aduh95 @juanarbol
test: improve FileHandle.prototype.write coverage
3734b64 
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
@aduh95 @juanarbol
test: improve FileHandle.prototype.write coverage
9055457 
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
@aduh95 @xtx1130
test: improve FileHandle.prototype.write coverage
ebefeb8 
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
@aduh95 @juanarbol
test: improve FileHandle.prototype.write coverage
7bf8117 
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
@aduh95 @danielleadams
test: improve FileHandle.prototype.write coverage
4e29299 
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
@aduh95 @targos
test: improve FileHandle.prototype.write coverage
c266559 
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
@aduh95 @targos
test: improve FileHandle.prototype.write coverage
a7b865c 
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
@aduh95 @guangwong
test: improve FileHandle.prototype.write coverage
5a4535f 
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
Reviewers

@mscdex mscdex mscdex requested changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alsoamit @aduh95 @mscdex @nodejs-github-bot