★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22024Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Conversation

Reviewers

@Trott Trott

@lpinca lpinca

@gireeshpunathil gireeshpunathil

@BridgeAR BridgeAR

@vsemozhetbyt vsemozhetbyt

@richardlau richardlau

Assignees
No one assigned
Labels
author ready doc fast-track
Projects
None yet
Milestone
No milestone
7 participants
@Maledong @Trott @richardlau @gireeshpunathil @vsemozhetbyt @lpinca @BridgeAR
@Maledong
Copy link
Member

Maledong commented Jul 30, 2018 •
edited

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 reviewed Jul 30, 2018
View changes

Copy link
Member

Trott left a comment •
edited

"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.)
@richardlau

richardlau reviewed Jul 30, 2018
View changes

lib/internal/readme.md Outdated
@@ -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

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Jul 30, 2018

Member

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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

richardlau commented Jul 30, 2018

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

@Maledong Maledong force-pushed the Maledong:FixTypoOfPlural branch from c04e6d9 to e02dfca Jul 30, 2018

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

@Maledong Maledong force-pushed the Maledong:FixTypoOfPlural branch from e02dfca to 1a2c0e1 Jul 30, 2018

@Maledong

This comment has been minimized.

Sign in to view
Copy link
Member Author

Maledong 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

This comment has been minimized.

Sign in to view
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.

@Maledong Maledong 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 Jul 30, 2018

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

@Maledong
doc: clarify text about internal module changes 
Simplify phrasing for clarity and succinctness.
Loading status checks…
311b039

@Maledong Maledong force-pushed the Maledong:FixTypoOfPlural branch from 1a2c0e1 to 311b039 Jul 30, 2018

@Maledong

This comment has been minimized.

Sign in to view
Copy link
Member Author

Maledong commented Jul 30, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Jul 30, 2018

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.
@gireeshpunathil

gireeshpunathil approved these changes Jul 30, 2018
View changes

@Trott

Trott approved these changes Jul 30, 2018
View changes

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 30, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/381/
@vsemozhetbyt

vsemozhetbyt approved these changes Jul 30, 2018
View changes

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 30, 2018

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

BridgeAR approved these changes Jul 30, 2018
View changes

@lpinca

lpinca approved these changes Jul 30, 2018
View changes

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 30, 2018

@richardlau Do you still object to this change?

@richardlau richardlau dismissed their stale review Jul 30, 2018

outdated

@vsemozhetbyt vsemozhetbyt added doc fast-track author ready labels Jul 31, 2018

@vsemozhetbyt

This comment has been minimized.

Sign in to view
Copy link
Member

vsemozhetbyt commented Jul 31, 2018

Landed in be322bd
Thank you!

@vsemozhetbyt vsemozhetbyt closed this Jul 31, 2018

vsemozhetbyt added a commit that referenced this pull request Jul 31, 2018

@Maledong @vsemozhetbyt
doc: clarify text about internal module changes 
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>
Loading status checks…
be322bd

targos added a commit that referenced this pull request Jul 31, 2018

@Maledong @targos
doc: clarify text about internal module changes 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
f4b6031

@Maledong Maledong deleted the Maledong:FixTypoOfPlural branch Jul 31, 2018

@Maledong

This comment has been minimized.

Sign in to view
Copy link
Member Author

Maledong commented Jul 31, 2018

Thanks all!

@targos targos referenced this pull request Jul 31, 2018

Merged

v10.8.0 proposal #22040

This was referenced Aug 1, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.