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: clarify usage of util.promisify.custom #16134

Closed
wants to merge 2 commits into from

Conversation

shiya
Copy link
Contributor

@shiya shiya commented Oct 11, 2017

Hoping this addition will help people using util.promisify.custom without having to read the source code.

Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Oct 11, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you Shiya! 💙

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

doc/api/util.md Outdated
@@ -525,6 +525,16 @@ console.log(promisified === doSomething[util.promisify.custom]);
This can be useful for cases where the original function does not follow the
standard format of taking an error-first callback as the last argument.

For example, if you have a pattern that takes in `(foo, onSuccessCallback, onErrorCallback)`, you can do:
Copy link
Member

Choose a reason for hiding this comment

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

Couple nits: please avoid the use of the informal pronoun you in the docs. In fact, I would just remove everything after For example:

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Left a comment, just needs one small tweak

@shiya shiya closed this Oct 13, 2017
@shiya shiya deleted the custom-promisify branch October 13, 2017 05:24
@shiya shiya reopened this Oct 13, 2017
@shiya
Copy link
Contributor Author

shiya commented Oct 13, 2017

Somehow messed up the commits... Should I close this PR and open a new one?

shiya added 2 commits October 13, 2017 13:27
Per requested changes. Since the code will only work for the
described pattern.
@shiya
Copy link
Contributor Author

shiya commented Oct 13, 2017

Ah, fixed it 😅 learned something about rebase today. @jasnell can you review again at your leisure? Thanks!

jasnell pushed a commit that referenced this pull request Oct 13, 2017
PR-URL: #16134
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Thank you @shiya! Landed in cc258af!

@jasnell jasnell closed this Oct 13, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#16134
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #16134
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants