-
Notifications
You must be signed in to change notification settings - Fork 30k
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: remove use of you #28919
doc: remove use of you #28919
Conversation
We generally avoid the use of 'you'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Also, from 7e8ad9b:
doc/api/policy.md:Since a dependency can be redirected, you can provide attenuated or modified
doc/api/policy.md:forms of dependencies as fits your application. For example, you could log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trades a personal pronoun (casual style) for a construction that is hard to understand. I'm not sure that trade-off is worth it. I'll suggest something else in a moment. (EDIT: Looking more closely, it was already hard to understand. But if we're in there editing, might as well fix that.)
@@ -147,8 +147,8 @@ available to the module code. | |||
|
|||
N-API versions are additive and versioned independently from Node.js. | |||
Version 4 is an extension to version 3 in that it has all of the APIs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 4 is an extension to version 3 in that it has all of the APIs | |
Version 4 is an extension to version 3 in that it has all the APIs |
How about this or something similar to it?
|
(@mhdawson Let me know if I should go in and make desired changes myself and push to your branch. Happy to do it, but also happy to let you evaluate wording and choose the right one, etc.) |
(If anyone is following along this far, it's the "which are listed as supporting a later version" part of the sentence that made me unsure of meaning. I wasn't sure if it meant a later version of Node.js or a later version of N-API. Of course, Node.js supporting a later version of Node.js makes no sense, but that just made me think there was a typo or a missing word or something.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided that my suggestions are non-blocking. They can be done additively in another PR. I'd prefer to just see them incorporated here but whatever. Current change looks good to me after all. Clearly I need to stop reviewing PRs when it's late and I'm tired. Because these lengthy multi-comment monologues are the result.
Landed in e3f4ec9 |
We generally avoid the use of 'you'. PR-URL: nodejs#28919 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
We generally avoid the use of 'you'. PR-URL: #28919 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
We generally avoid the use of 'you'.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes