GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — docs: improve promise terminology by benjamingr · Pull Request #37181 · nodejs/node

Conversation

@benjamingr

@benjamingr benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@benjamingr
docs: improve promise terminology
fb170f6
@benjamingr benjamingr added the doc Issues and PRs related to Node.js documentation. label Feb 2, 2021
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Feb 2, 2021
aduh95
aduh95 approved these changes Feb 2, 2021
View reviewed changes
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 2, 2021
@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Feb 2, 2021

Copy link
Copy Markdown
Contributor

I thought author-ready implies CI started though?

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

node/doc/guides/collaborator-guide.md

Line 217 in 9cf2341

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

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
Member Author

Good to know, thanks 🙏

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Lxxyx
Lxxyx approved these changes Feb 2, 2021
View reviewed changes
RaisinTen
RaisinTen approved these changes Feb 2, 2021
View reviewed changes
@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
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, benjamingr, and Lxxyx reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RaisinTen

RaisinTen commented Feb 2, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
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).
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr benjamingr added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Feb 2, 2021
@benjamingr

benjamingr commented Feb 2, 2021

Copy link
Copy Markdown
Member Author

Landed in 271e04f

RaisinTen reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr benjamingr closed this Feb 2, 2021
benjamingr added a commit that referenced this pull request Feb 2, 2021
@benjamingr
doc: improve promise terminology
271e04f
PR-URL: #37181
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@RaisinTen

RaisinTen commented Feb 2, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@benjamingr @danielleadams
doc: improve promise terminology
67c9a8e
PR-URL: #37181
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
This was referenced Feb 16, 2021
v15.9.0 proposal #37405
Closed
v15.9.0 proposal #37406
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@benjamingr @targos
doc: improve promise terminology
11d3e71
PR-URL: #37181
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lxxyx Lxxyx Lxxyx approved these changes
@aduh95 aduh95 aduh95 approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. fast-track PRs proposed for a shorter-than-standard waiting period before landing. fs Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@benjamingr @aduh95 @RaisinTen @Lxxyx @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.