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

doc: improve fsPromises docs #37186

Closed
wants to merge 1 commit into from
Closed

doc: improve fsPromises docs #37186

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

Improe the filesystem promises docs:

  • Clarify promise terminology (promises are never fulfilled without a value - just like functions in JS).
  • Add some links and parameter descriptions.

I wanted to put all the changes here - but this PR got pretty big pretty fast so I figured I'd split the changes into multiple smaller PRs to make review reasonable and reduce the amount of potential conflicts.

CC @RaisinTen

@benjamingr benjamingr added the doc Issues and PRs related to the documentations. label Feb 2, 2021
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 2, 2021
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 2, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 2, 2021

Linter is failing.

@benjamingr
Copy link
Member Author

At some day - I will figure out a workflow that will let me deal with the fact Node is the one project I often force-push to :D and I'll stop forgetting to check I did not get the "Updates were rejected" message.

@benjamingr benjamingr mentioned this pull request Feb 2, 2021
Closed
RaisinTen
RaisinTen reviewed Feb 2, 2021
View reviewed changes
doc/api/fs.md

`length` is an integer specifying the number of bytes to read.

`position` is an argument specifying where to begin reading from in the file.
If `position` is `null`, data will be read from the current file position,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If `position` is `null`, data will be read from the current file position,
If `position` is `null` or `-1`, data will be read from the current file position,

@jasnell
Copy link
Member

jasnell commented Feb 2, 2021

@benjamingr ... could these changes be bundled in with this PR instead to prevent conflicts? #37170

@benjamingr
Copy link
Member Author

@jasnell sure.

I'd also like to apologize for about landing the resolved->fulfilled changes I would have waited with those as well if I would have seen your PR.

It would be super useful to have automation to tell you if changes you are proposing conflict with other open PRs.

@benjamingr
Copy link
Member Author

@jasnell Would you prefer it if I pushed the changes to your branch or wait for it to land before re-opening this PR?

@benjamingr benjamingr closed this Feb 2, 2021
@jasnell
Copy link
Member

jasnell commented Feb 2, 2021

Feel free to push to my branch if you'd like.

@jasnell
Copy link
Member

jasnell commented Feb 2, 2021

And no worries about the conflicts. We can get that rebased pretty easily

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benjamingr @aduh95 @jasnell @RaisinTen @nodejs-github-bot