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

docs: improve promise terminology #37181

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

This is kind of a nit but:

A promise is:

  • resolved if its fate is known either because it is tracking another promise or because it is fulfilled with a value. For example const p = Promise.resolve(timersPromises.setTimeout(1000)); - here p is a resolved promise. The promise constructor calls its executor argument function resolve for this case since it performs resolution and you can pass it a promise and it'll track it.
  • fulfilled means what the docs mean when they use "resolved". It means "this promise has settled on a value successfully". p for the example above has not fulfilled (immediately).

@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 the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 2, 2021
@benjamingr
Copy link
Member Author

@aduh95 thanks, (I thought author-ready implies CI started though?)

@aduh95
Copy link
Contributor

aduh95 commented Feb 2, 2021

I thought author-ready implies CI started though?

For doc-only changes, GH Actions are enough to land.

For documentation-only changes, GitHub Actions CI is sufficient.

@benjamingr
Copy link
Member Author

Good to know, thanks 🙏

@benjamingr
Copy link
Member Author

Apparently this is also fine to fast-track according to:

Focused changes that affect only documentation

So 👍 to fast track if you think this is fine to fast-track :]

@RaisinTen
Copy link
Contributor

nit: then fulfills feels a little abrupt in these sentences. Should we prepend an and or a , to make it sound better?

@benjamingr
Copy link
Member Author

@RaisinTen I don't mind making a follow up PR auditing the documentation further - I think certain ways we phrase things isn't really super helpful:

For example stuff like

#### `filehandle.chown(uid, gid)`
<!-- YAML
added: v10.0.0
-->

* `uid` {integer}
* `gid` {integer}
* Returns: {Promise}

Changes the ownership of the file then fulfills the `Promise` with no arguments
upon success.

Probably should be:

#### `filehandle.chown(uid, gid)`
<!-- YAML
added: v10.0.0
-->

* `uid` {integer} The file's new owner's user id.
* `gid` {integer} The file's new group's group id.
* Returns: {Promise} Fulfills with `undefined` when the operation has completed.

Changes the ownership of the given file. A wrapper for chown(2).

@benjamingr
Copy link
Member Author

If I make such a PR would you be up for reviewing?

@benjamingr benjamingr added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 2, 2021
@benjamingr
Copy link
Member Author

Landed in 271e04f

@benjamingr benjamingr closed this Feb 2, 2021
benjamingr added a commit that referenced this pull request Feb 2, 2021
PR-URL: #37181
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor

@benjamingr that looks much better. Sure, I will. :)

@benjamingr benjamingr deleted the doc-promise-terminology branch February 2, 2021 13:28
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37181
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This was referenced Feb 16, 2021
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #37181
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 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. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants