doc: Update fs.read() documentation for clarity by mertcanaltin · Pull Request #52453 · nodejs/node

mertcanaltin · 2024-04-10T18:50:23Z

This PR updates the documentation to clarify the behavior of Node.js's fs.read() method. It was noticed that there are ambiguities regarding the function of the length argument in the current documentation. The newly added explanations emphasize that the length argument specifies the maximum number of bytes to be read and that the actual number of bytes read may be less than this value. This update will help developers better understand and utilize the fs.read() method.

Issue #52447

LiviaMedeiros · 2024-04-10T20:28:03Z

It's described as fs.ftruncate(fd[, len], callback) in docs, which already means that fd and callback are mandatory. Would you like to elaborate why this is needed?
Also, the linked issue doesn't look related.

mertcanaltin · 2024-04-11T07:03:01Z

It's described as fs.ftruncate(fd[, len], callback) in docs, which already means that fd and callback are mandatory. Would you like to elaborate why this is needed? Also, the linked issue doesn't look related.

Yes, I misunderstood, I tried to read better and make a document (I commented that we should specify the callback parameter)

mertcanaltin · 2024-04-11T07:15:51Z

I did an update and I wonder if I'm on the right track @LiviaMedeiros

doc/api/fs.md

mcollina

lgtm

doc/api/fs.md

mcollina

lgtm

nodejs-github-bot · 2024-04-24T15:55:59Z

Commit Queue failed

- Loading data for nodejs/node/pull/52453
✔  Done loading data for nodejs/node/pull/52453
----------------------------------- PR info ------------------------------------
Title      doc: Update fs.read() documentation for clarity (#52453)
Author     Mert Can Altin  (@mertcanaltin)
Branch     mertcanaltin:dev-52447 -> nodejs:main
Labels     doc, fs, commit-queue-squash
Commits    8
 - doc: update fs.read() documentation for clarity
 - Update fs.md
 - update fs.read() documentation with clarification on EOF handling
 - Merge branch 'dev-52447' of https://github.com/mertcanaltin/node into…
 - unnecessary space removed
 - fix: statements organized
 - fix: statements organized
 - docs: Update fs.read() documentation for clarity
Committers 2
 - Mert Can Altin 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/52453
Reviewed-By: Matteo Collina 
Reviewed-By: Marco Ippolito 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52453
Reviewed-By: Matteo Collina 
Reviewed-By: Marco Ippolito 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 10 Apr 2024 18:50:23 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52453#pullrequestreview-2020283381
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/52453#pullrequestreview-1999533582
   ⚠  GitHub cannot link the author of 'doc: update fs.read() documentation for clarity' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'update fs.read() documentation with clarification on EOF handling' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'Merge branch 'dev-52447' of https://github.com/mertcanaltin/node into…' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'unnecessary space removed' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'fix: statements organized' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'fix: statements organized' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'docs: Update fs.read() documentation for clarity' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 52453
From https://github.com/nodejs/node
 * branch                  refs/pull/52453/merge -> FETCH_HEAD
✔  Fetched commits as ac9aa37bcb5e..83886ed9062f
--------------------------------------------------------------------------------
[main 5331638bd1] doc: update fs.read() documentation for clarity
 Author: Mert Can Altin 
 Date: Thu Apr 11 10:54:23 2024 +0300
 1 file changed, 26 insertions(+)
[main 6833765805] Update fs.md
 Author: Mert Can Altin 
 Date: Thu Apr 11 11:34:06 2024 +0300
 1 file changed, 1 insertion(+), 1 deletion(-)
Auto-merging doc/api/fs.md
error: commit ca223a3f1d636b1bcbc0ea5fed6c0bca3f82ccca is a merge but no -m option was given.
fatal: cherry-pick failed
[main da4e265036] update fs.read() documentation with clarification on EOF handling
 Author: Mert Can Altin 
 Date: Sun Apr 14 14:59:44 2024 +0300
 1 file changed, 3 insertions(+), 1 deletion(-)
   ✘  Failed to apply patches

https://github.com/nodejs/node/actions/runs/8819523502

LiviaMedeiros

This is unrelated to fs.ftruncate() and must be moved to fs.read() part of documentation before merging.

mertcanaltin · 2024-04-24T16:01:21Z

This is unrelated to fs.ftruncate() and must be moved to fs.read() part of documentation before merging.

do you have a chance to send a change proposal I'm a little confused @LiviaMedeiros

LiviaMedeiros · 2024-04-25T20:31:53Z

fs.read(fd, buffer, offset, length, position, callback) (where the change belongs) is there. This is fs.ftruncate(fd[, len], callback).

mertcanaltin · 2024-04-27T09:45:43Z

fs.read(fd, buffer, offset, length, position, callback) (where the change belongs) is there. This is fs.ftruncate(fd[, len], callback).

I updated as you said thank you very much ❤️ 🙏

resolved

nodejs-github-bot · 2024-05-10T18:04:07Z

Landed in a27f473

PR-URL: #52453 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#52453 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Apr 10, 2024

mertcanaltin changed the title ~~doc: updated fs.ftruncate() docs to reflect mandatory callback parameter~~ doc: Update fs.read() documentation for clarity Apr 11, 2024

mertcanaltin force-pushed the dev-52447 branch from 3ce187a to ab76d28 Compare April 11, 2024 07:14

mertcanaltin requested a review from LiviaMedeiros April 11, 2024 07:15

mertcanaltin force-pushed the dev-52447 branch from ab76d28 to 3cc3647 Compare April 11, 2024 07:54

meyfa reviewed Apr 11, 2024

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mertcanaltin requested review from joyeecheung, marco-ippolito, mcollina and targos April 14, 2024 09:08

mcollina approved these changes Apr 14, 2024

View reviewed changes

marco-ippolito approved these changes Apr 14, 2024

View reviewed changes

LiviaMedeiros reviewed Apr 14, 2024

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

doc/api/fs.md Outdated Show resolved Hide resolved

mertcanaltin requested a review from LiviaMedeiros April 14, 2024 20:28

LiviaMedeiros reviewed Apr 14, 2024

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

LiviaMedeiros reviewed Apr 14, 2024

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

targos removed their request for review April 15, 2024 08:13

mertcanaltin requested a review from LiviaMedeiros April 15, 2024 15:21

mcollina reviewed Apr 19, 2024

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

mertcanaltin requested a review from mcollina April 23, 2024 08:10

mcollina approved these changes Apr 24, 2024

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 24, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 24, 2024

LiviaMedeiros previously requested changes Apr 24, 2024

View reviewed changes

mertcanaltin force-pushed the dev-52447 branch from 43e9a13 to 4198867 Compare April 27, 2024 09:44

mertcanaltin requested a review from LiviaMedeiros April 27, 2024 09:45

doc: update fs read documentation for clarity

01d8a11

mertcanaltin force-pushed the dev-52447 branch from 4198867 to 01d8a11 Compare April 27, 2024 10:01

jasnell approved these changes Apr 27, 2024

View reviewed changes

mertcanaltin added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2024

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 10, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 10, 2024

nodejs-github-bot merged commit a27f473 into nodejs:main May 10, 2024

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

LiviaMedeiros mentioned this pull request May 16, 2024

What does fs.read's length argument indicate? #52447

Closed

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

Uh oh!

Conversation

mertcanaltin commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiviaMedeiros commented Apr 10, 2024

Uh oh!

mertcanaltin commented Apr 11, 2024

Uh oh!

mertcanaltin commented Apr 11, 2024

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 24, 2024

Uh oh!

LiviaMedeiros left a comment

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Apr 24, 2024

Uh oh!

LiviaMedeiros commented Apr 25, 2024

Uh oh!

mertcanaltin commented Apr 27, 2024

Uh oh!

nodejs-github-bot commented May 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mertcanaltin commented Apr 10, 2024 •

edited

Loading