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

doc: Update fs.read() documentation for clarity #52453

Merged
merged 1 commit into from
May 10, 2024

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Apr 10, 2024

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

@nodejs-github-bot 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
@LiviaMedeiros
Copy link
Contributor

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
Copy link
Member Author

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 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
Copy link
Member Author

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

doc/api/fs.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@targos targos removed their request for review April 15, 2024 08:13
doc/api/fs.md Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina 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 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
@nodejs-github-bot
Copy link
Collaborator

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

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

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

@mertcanaltin
Copy link
Member Author

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
Copy link
Contributor

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

@mertcanaltin
Copy link
Member Author

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 ❤️ 🙏

@mertcanaltin 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 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 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 nodejs-github-bot merged commit a27f473 into nodejs:main May 10, 2024
20 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a27f473

targos pushed a commit that referenced this pull request May 11, 2024
PR-URL: #52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
PR-URL: nodejs#52453
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants