FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: update promisify() docs with behavior when bad arguments are passed by lossycache · Pull Request #17593 · nodejs/node · GitHub

/ node Public

doc: update promisify() docs with behavior when bad arguments are passed - #17593

Closed
lossycache wants to merge 3 commits into
nodejs:masterfrom
lossycache:doc-promisify-update
ConversationCommits (3)ChecksFiles changed
Closed

doc: update promisify() docs with behavior when bad arguments are passed#17593
lossycache wants to merge 3 commits into
nodejs:masterfrom
lossycache:doc-promisify-update

Conversation

lossycache commented Dec 10, 2017

Copy link
Copy Markdown
Contributor

Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not necessarily
the case, since the behavior is well defined, but just not useful.

Fixes: #17569

Checklist
Affected core subsystem(s)

doc

nodejs-github-bot added doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Dec 10, 2017
lossycache changed the title Update promisify() docs with behavior when bad arguments are passed doc: update promisify() docs with behavior when bad arguments are passed Dec 10, 2017
apapirovski approved these changes Dec 10, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

SGTM — very concise. Just have a few nits below.

Comment thread doc/api/util.md Outdated

apapirovski Dec 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we tend to refer to these as "Node.js style callback".

Comment thread doc/api/util.md Outdated

apapirovski Dec 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the "anyways" shouldn't be necessary here.

Trott reviewed Dec 10, 2017
View reviewed changes
Comment thread doc/api/util.md Outdated

Trott Dec 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you or someone else would like a good second pull request after this one lands, I'd be all for getting rid of the awkward and non-obvious Node.js style callback terminology here and in errors.md and replacing it with error-first callback which is more informative, less awkward, and probably more in line with usage outside of our docs.

lpinca Dec 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: long line, please add a line break at 80 chars. Also there seems to be a redundant "be"?

Trott approved these changes Dec 10, 2017
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me! Thanks for the pull request!

benjamingr approved these changes Dec 11, 2017
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for doing this :)

maclover7 commented Dec 11, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/12/

lpinca approved these changes Dec 12, 2017
View reviewed changes
Comment thread doc/api/util.md Outdated

lpinca Dec 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: long line, please add a line break at 80 chars. Also there seems to be a redundant "be"?

doc: update promisify docs with behavior when bad arguments are passed
2067648 
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not necessarily
the case, since the behavior is well defined, but just not useful.

Fixes: nodejs#17569 (comment)
doc: use 'Node.js style' callback
1c272f9
doc: fix line length and wording
e215574

lossycache commented Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

@lpinca whoops, my bad! just noticed those details. Thanks for pointing them out! 👍

apapirovski added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 12, 2017

apapirovski commented Dec 12, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/15/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
doc: clearify promisify behavior for bad arguments
031d01b 
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not
necessarily the case, since the behavior is well defined, but just
not useful.

PR-URL: nodejs#17593
Fixes: nodejs#17569
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member

Landed in 031d01b

BridgeAR closed this Dec 12, 2017

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member

@ramsgoli just as a note - I fixed the commit message as it did not comply to our guidelines (that you ticket off while opening the PR). The commit message and the general line length was to long.

lossycache commented Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR ah, thank you so much for the reminder! I need to be sure to pay attention to all the details of the guidelines next time. Thanks again 🙌

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member

Sure thing. I just thought it is best to provide a short feedback so I have less work when landing your next PR 😄

lossycache mentioned this pull request Dec 12, 2017
Closed
2 tasks
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2017
lossycache added a commit to lossycache/node that referenced this pull request Dec 13, 2017
doc: change Node.js style to error-first style
324b87b 
We change the awkward "Node.js style callback" phrasing to the
more informative "error-first style callback," which is more
in line with its usage

Refs: nodejs#17593 (comment)
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
doc: clearify promisify behavior for bad arguments
da44821 
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not
necessarily the case, since the behavior is well defined, but just
not useful.

PR-URL: #17593
Fixes: #17569
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 22, 2018
doc: clearify promisify behavior for bad arguments
24e7753 
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not
necessarily the case, since the behavior is well defined, but just
not useful.

PR-URL: #17593
Fixes: #17569
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
apapirovski apapirovski approved these changes
maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL