FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed typo. Removed useless "?". by alsoamit · Pull Request #42518 · nodejs/node · GitHub

/ node Public

Fixed typo. Removed useless "?". - #42518

Closed
alsoamit wants to merge 1 commit into
nodejs:masterfrom
alsoamit:master
ConversationCommits (1)ChecksFiles changed
Closed

Fixed typo. Removed useless "?".#42518
alsoamit wants to merge 1 commit into
nodejs:masterfrom
alsoamit:master

Conversation

alsoamit commented Mar 30, 2022

Copy link
Copy Markdown

No description provided.

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

mscdex left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

Looks like tests are passing with this change

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

aduh95 commented Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 closed this Mar 30, 2022
aduh95 added a commit to aduh95/node that referenced this pull request Mar 31, 2022
test: improve FileHandle.prototype.write coverage
e588592 
Refs: nodejs#42518
aduh95 mentioned this pull request Mar 31, 2022
Merged
nodejs-github-bot pushed a commit that referenced this pull request Apr 2, 2022
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 mentioned this pull request Apr 3, 2022
Merged
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Apr 3, 2022
squash: remove unused optional chaining
ac6f1df 
Refs: nodejs#42518
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
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
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
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
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
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
mscdex mscdex requested changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL