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 text about internal module changes #22024

Closed
wants to merge 1 commit into from
Closed

doc: clarify text about internal module changes #22024

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 30, 2018

For a non-English native speaker it's hard to understand what it means.
So make it a common and more clearly speaking.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows [commit guidelines]

Trott
Trott previously requested changes Jul 30, 2018
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.

"These are subject to change" is correct. The word subject is a verb here, not a noun. A more wordy way of saying it would be "These may be subjected to changes". However, probably for historical reasons, subject to change is the natural, idiomatic way to say it for native speakers.

(Aside: This is actually really interesting for me. I've never thought twice about the phrase subject to change and so I'm now realizing that I never really thought about exactly what each word means.)

@@ -2,5 +2,5 @@

The modules in `lib/internal` are intended for internal use in Node.js core
only, and are not accessible with `require()` from user modules. These are
subject to change at **any** time. Reliance on these modules outside of core
subjects to change at **any** time. Reliance on these modules outside of core
Copy link
Member

Choose a reason for hiding this comment

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

subject to change is a phrase so this change is incorrect.

@Trott
Copy link
Member

Trott commented Jul 30, 2018

If you want to clarify it for people unfamiliar with the subject to change idiom, how about changing from this:

These are subject to change at any time.

...to this:

These can change at any time.

Seems both clearer and more succinct to me.

(I'm also 👍 on eliminating the emphasis on any if you want to do that too.)

@Trott
Copy link
Member

Trott commented Jul 30, 2018

And I'd also be 👍 on making it clear what these refers to, so maybe this?:

These modules can change at any time.

@richardlau
Copy link
Member

(Aside: This is actually really interesting for me. I've never thought twice about the phrase subject to change and yet I'm now realizing that I never really thought about exactly what each word means.)

I think this is one of those times that being a native speaker is a disadvantage when trying to explain things. You know whether a phrase is correct/incorrect but you're not exactly sure how to explain why.

@ghost ghost changed the title doc: Fix typo of plural "subject" doc: Make the native speaker more clearly Jul 30, 2018
@ghost
Copy link
Author

ghost commented Jul 30, 2018

@Trott & @richardlau
It's not the problem whether you are using a native English statement or not, but just as what you said above, this isn't clear so I feel a little missed up and puzzled. So I've changed according to what you suggested to me, which makes it clearer and better.

@Trott
Copy link
Member

Trott commented Jul 30, 2018

The commit message is hard to understand. How about this?:

doc: clarify text about internal module changes

Simplify phrasing for clarity and succinctness.

@ghost ghost changed the title doc: Make the native speaker more clearly doc: clarify text about internal module changes Jul 30, 2018
@Trott Trott dismissed their stale review July 30, 2018 03:28

no objection to new change; holding off on approving until commit message is 👍

Simplify phrasing for clarity and succinctness.
@ghost
Copy link
Author

ghost commented Jul 30, 2018

No problem. For a Node lover I hope I can make it better according to our suggestions and ideas.

@gireeshpunathil
Copy link
Member

Looks like a thin boundary between clarity and expressiveness. Agreeing to all the comments above, and in addition acknowledging the doc improvement if it helps easy consumption for wider audience.

@Trott
Copy link
Member

Trott commented Jul 30, 2018

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt
Copy link
Contributor

@richardlau Do you still object to this change?

@richardlau richardlau dismissed their stale review July 30, 2018 22:42

outdated

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 31, 2018
@vsemozhetbyt
Copy link
Contributor

Landed in be322bd
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Jul 31, 2018
Simplify phrasing for clarity and succinctness.

PR-URL: #22024
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2018
Simplify phrasing for clarity and succinctness.

PR-URL: #22024
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ghost ghost deleted the FixTypoOfPlural branch July 31, 2018 07:56
@ghost
Copy link
Author

ghost commented Jul 31, 2018

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants