-
-
Notifications
You must be signed in to change notification settings - Fork 70
docs: add up to date documentation for AsyncWrap #27
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
Conversation
@trevnorris can you review? |
docs/AsyncWrap/README.md
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@AndreasMadsen Thanks for all the text. I think my referenced patch is the last for the near future. There are some grammatical things, but we'll worry about those later. |
docs/AsyncWrap/README.md
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@trevnorris Thanks for the review. I have updated the documentation based on your comments and nodejs/node#3216 (add |
docs/AsyncWrap/README.md
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Thanks @Qard, I have updated the documentation. |
I assume we're ready to merge this? Or are more updates coming? |
docs/AsyncWrap/README.md
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
finished another round. mainly grammer and stuff. great stuff. |
Thanks @trevnorris. |
docs/AsyncWrap/README.md
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Two nits. Other than that I would say LGTM. Nice bit of writing you did! |
Fixed. |
LGTM |
docs: add up to date documentation for AsyncWrap
This should be an up to date documentation on the AsyncWrap API. I have not included in discussion on the future of the API, but there a small section (Things you might not expect) on some stuff that other users should know.
Also I'm not really sure what the purpose of the provider argument is and how it compares to
this.constructor.name
someone else should elaborate on that.PS: I've heard my spelling isn't ideal, bear with me.