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: improve fs doc intro #34843

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 19, 2020

/cc @Trott

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@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 Aug 19, 2020
doc/api/fs.md Outdated Show resolved Hide resolved
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2020
doc/api/fs.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Aug 20, 2020

I think the comparison between "asynchronous" and Promise-based APIs could be confusing. The Promise-based APIs definitely are asynchronous, they're just not the callback-based API.

@jasnell
Copy link
Member Author

jasnell commented Aug 20, 2020

@mscdex updated to address your concern

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
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
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
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
doc/api/fs.md Outdated Show resolved Hide resolved
@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2020
doc/api/fs.md Outdated Show resolved Hide resolved
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2020
doc/api/fs.md Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Aug 21, 2020

Not blocking on it, but I'm 👎 on the capitalization in Promise-based. The only justification I can think of is that it emphasizes the underlying Promise class. I think that draws attention to the wrong thing (implementation detail) in promise-based.

For what it's worth, MDN agrees, based on the contents of https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Asynchronous/Promises (where promise-based appears 7 times and Promise-based not at all).

I also interpret the Microsoft style guide section on capitalization as falling on the "when in doubt, don't capitalize" side of things.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2020
@Trott
Copy link
Member

Trott commented Aug 23, 2020

I removed the author ready label in light of lpinca's comment and my comment, but I think this is ready to land. I don't think either of those comments are blocking (I know mine isn't and they approved the PR despite the comment they left suggesting a chance.)

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 24, 2020
Signed-off-by: James M Snell <[email protected]>
@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 24, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 24, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/34843
✔  Done loading data for nodejs/node/pull/34843
----------------------------------- PR info ------------------------------------
Title      doc: improve fs doc intro (#34843)
Author     James M Snell  (@jasnell)
Branch     jasnell:new-fs-doc-improvements -> nodejs:master
Labels     author ready, doc, fs
Commits    1
 - doc: improve fs doc intro
Committers 1
 - James M Snell 
PR-URL: https://github.com/nodejs/node/pull/34843
Reviewed-By: Anna Henningsen 
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34843
Reviewed-By: Anna Henningsen 
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: improve fs doc intro
   ✖  No CI runs detected
   ℹ  This PR was created on Wed, 19 Aug 2020 19:59:53 GMT
   ✔  Approvals: 4
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/34843#pullrequestreview-470845622
   ✔  - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/34843#pullrequestreview-471545184
   ✔  - Anto Aravinth (@antsmartian): https://github.com/nodejs/node/pull/34843#pullrequestreview-472057857
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/34843#pullrequestreview-472762285
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 24, 2020
@jasnell
Copy link
Member Author

jasnell commented Aug 24, 2020

Landed in 010383a

@jasnell jasnell closed this Aug 24, 2020
jasnell added a commit that referenced this pull request Aug 24, 2020
Signed-off-by: James M Snell <[email protected]>

PR-URL: #34843
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Aug 25, 2020
Signed-off-by: James M Snell <[email protected]>

PR-URL: #34843
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danielleadams danielleadams mentioned this pull request Aug 25, 2020
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Signed-off-by: James M Snell <[email protected]>

PR-URL: #34843
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Signed-off-by: James M Snell <[email protected]>

PR-URL: #34843
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
richardlau added a commit to richardlau/node-1 that referenced this pull request Oct 22, 2020
nodejs#34843 was cherry-picked onto
`v12.x-staging` in 961844d but the `fs/promises` API, as used
in the examples, is only available from Node.js 14. Change the
added examples to use `require(fs).promises` instead.
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
#34843 was cherry-picked onto
`v12.x-staging` in 961844d but the `fs/promises` API, as used
in the examples, is only available from Node.js 14. Change the
added examples to use `require(fs).promises` instead.

PR-URL: #35760
Fixes: #35740
Refs: #34843
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
#34843 was cherry-picked onto
`v12.x-staging` in 961844d but the `fs/promises` API, as used
in the examples, is only available from Node.js 14. Change the
added examples to use `require(fs).promises` instead.

PR-URL: #35760
Fixes: #35740
Refs: #34843
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
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. 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.

10 participants