GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix fs.read documentation typo read instead of write by HACHIMIam · Pull Request #29270 · nodejs/node

Conversation

@HACHIMIam

@HACHIMIam HACHIMIam commented Aug 22, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and ouadie-lahdioui reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@hachimiSfeir
fix fs.read documentation typo read instead of write
58c2ab7
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. fs Issues and PRs related to file-system APIs and the fs module. labels Aug 22, 2019
@HACHIMIam HACHIMIam mentioned this pull request Aug 22, 2019
fs.read documentation errors #29266
Closed
ronag
ronag requested changes Aug 22, 2019
View reviewed changes
Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@HACHIMIam @ronag
Update doc/api/fs.md
2fa552c 
Co-Authored-By: Robert Nagy <[email protected]>
ronag
ronag approved these changes Aug 22, 2019
View reviewed changes
trivikr
trivikr approved these changes Aug 23, 2019
View reviewed changes
@trivikr

trivikr commented Aug 23, 2019

Copy link
Copy Markdown
Member

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3850/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell previously approved these changes Aug 23, 2019
View reviewed changes
@trivikr trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 23, 2019
lpinca
lpinca previously approved these changes Aug 23, 2019
View reviewed changes
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 25, 2019
addaleax
addaleax requested changes Aug 25, 2019
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasnell @trivikr @lpinca Are you sure this is more understandable than before? The data will be read from the disk and written into the buffer (i.e. the access to the buffer memory is a write-only one), so I’m not sure that we should land this as-is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@HACHIMIam @addaleax
Update doc/api/fs.md
9c6068d 
Co-Authored-By: Anna Henningsen <[email protected]>
@trivikr
trivikr requested a review from addaleax August 27, 2019 14:15
@trivikr

trivikr commented Aug 27, 2019

Copy link
Copy Markdown
Member

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3863/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Aug 27, 2019
View reviewed changes
Comment thread doc/api/fs.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 28, 2019

Copy link
Copy Markdown
Member

@addaleax I'm good with the before or after. If you think the original reads better then I'm good with that also.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Aug 28, 2019

Copy link
Copy Markdown
Member

I still feel like the second line change here sounds a lot like implying that the Buffer is the data source here. It can be reading into if that’s better, but reading from really doesn’t work here, imo.
jasnell and BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 28, 2019

Copy link
Copy Markdown
Member

As I said, i'm good with your read on it @addaleax. I'll clear my +1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell dismissed their stale review August 28, 2019 23:15

Happy with @addaleax's rejection and reasoning

@lpinca
lpinca dismissed their stale review September 9, 2019 16:30

I agree with @addaleax. Sorry I did not review carefully.

@Trott
Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51
@BridgeAR

BridgeAR commented Jan 12, 2020

Copy link
Copy Markdown
Member

Ping @HACHIMIam would you please change the second part as suggested?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hachimiSfeir
keep writing at in offset params in fs.read
a6387e4
@HACHIMIam

HACHIMIam commented Jan 12, 2020

Copy link
Copy Markdown
Author

@BridgeAR done
BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BridgeAR
BridgeAR approved these changes Jan 12, 2020
View reviewed changes
BridgeAR
BridgeAR reviewed Jan 12, 2020
View reviewed changes
Comment thread doc/api/fs.md Outdated
Read data from the file specified by `fd`.

`buffer` is the buffer that the data will be written to.
`buffer` is the buffer that the data which was read from the fd will be written to.

@BridgeAR BridgeAR Jan 12, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This line exceeds the maximum allowed line length.

Suggested change
`buffer` is the buffer that the data which was read from the fd will be written to.
`buffer` is the buffer that the data which was read from the fd will be written
to.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@HACHIMIam HACHIMIam Jan 14, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@BridgeAR i tried reduce the line length

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
addaleax
addaleax approved these changes Jan 12, 2020
View reviewed changes
cjihrig
cjihrig approved these changes Jan 12, 2020
View reviewed changes
@hachimiSfeir
reduce fs.read buffer line size
6e2ab87
@HarshithaKP

HarshithaKP commented Apr 1, 2020

Copy link
Copy Markdown
Member

Looks like this was ready to be landed long back, but not sure why got stalled.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gireeshpunathil gireeshpunathil added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 1, 2020
@gireeshpunathil

gireeshpunathil commented Apr 1, 2020

Copy link
Copy Markdown
Member

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4157
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Apr 2, 2020

Copy link
Copy Markdown
Member

Landed in 0c70e8c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Apr 2, 2020
addaleax pushed a commit that referenced this pull request Apr 2, 2020
@hachimiSfeir @addaleax
doc: improve fs.read documentation
0c70e8c 
PR-URL: #29270
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@hachimiSfeir @BethGriggs
doc: improve fs.read documentation
1ad6995 
PR-URL: #29270
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
@hachimiSfeir @targos
doc: improve fs.read documentation
b6f7196 
PR-URL: #29270
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
v13.13.0 release proposal #32813
Merged
targos pushed a commit that referenced this pull request Apr 22, 2020
@hachimiSfeir @targos
doc: improve fs.read documentation
4b417f8 
PR-URL: #29270
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Apr 22, 2020
v12.16.3 release proposal #33009
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronag ronag ronag approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@trivikr trivikr trivikr approved these changes
@jasnell jasnell jasnell left review comments
@lpinca lpinca lpinca left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. fs Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@HACHIMIam @trivikr @jasnell @addaleax @BridgeAR @HarshithaKP @gireeshpunathil @lpinca @cjihrig @ronag @nodejs-github-bot @hachimiSfeir
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.