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: remove use of you #28919

Closed
wants to merge 1 commit into from
Closed

doc: remove use of you #28919

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Aug 1, 2019

We generally avoid the use of 'you'.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

We generally avoid the use of 'you'.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Aug 1, 2019
Copy link
Member

@ChALkeR ChALkeR left a 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

Copy link
Member

@Trott Trott left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

@Trott
Copy link
Member

Trott commented Aug 2, 2019

How about this or something similar to it?

This means that it is not necessary to recompile for new versions of Node.js. For example, Node.js 10.00 supports N-API 3. It is also compatible with addons compiled with N-API 2 and N-API 1, but not N-API 4.

@Trott
Copy link
Member

Trott commented Aug 2, 2019

(@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.)

@Trott
Copy link
Member

Trott commented Aug 2, 2019

(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.)

Copy link
Member

@Trott Trott left a 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.

@Trott
Copy link
Member

Trott commented Aug 4, 2019

@Trott
Copy link
Member

Trott commented Aug 4, 2019

Landed in e3f4ec9

@Trott Trott closed this Aug 4, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 4, 2019
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>
BridgeAR pushed a commit that referenced this pull request Aug 6, 2019
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>
@mhdawson mhdawson deleted the doc-update branch September 14, 2020 21:18
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. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants