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.

Sign up for GitHub

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

Jump to bottom

doc: improve process.platform #18057

Closed
wants to merge 1 commit into from
Closed

doc: improve process.platform #18057

wants to merge 1 commit into from

Conversation

marswong
Copy link
Contributor

@marswong marswong commented Jan 9, 2018

Checklist
  • 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
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Jan 9, 2018
Fishrock123
Fishrock123 reviewed Jan 9, 2018
View reviewed changes
doc/api/process.md Outdated

```js
console.log(`This platform is ${process.platform}`);
```

*Note*: The value `'android'` may also be returned if the Node.js is built on
the Android operating system. However, Android support in Node.js is considered
to be experimental at this time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we link to the supported platforms table? (I forget where it is..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find that either...

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/nodejs/node/blob/master/BUILDING.md#supported-platforms-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gibfahn it seems that the table just list the supported platforms, but not the value of the API.

Trott
Trott reviewed Jan 10, 2018
View reviewed changes
doc/api/process.md Outdated

```js
console.log(`This platform is ${process.platform}`);
```

*Note*: The value `'android'` may also be returned if the Node.js is built on
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking nit: Remove *Note*: and let the paragraph stand on its own.

Trott
Trott reviewed Jan 10, 2018
View reviewed changes
doc/api/process.md Outdated

```js
console.log(`This platform is ${process.platform}`);
```

*Note*: The value `'android'` may also be returned if the Node.js is built on
the Android operating system. However, Android support in Node.js is considered
to be experimental at this time.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: is considered to be experimental at this time -> is experimental.

Trott
Trott reviewed Jan 11, 2018
View reviewed changes
doc/api/process.md Outdated

```js
console.log(`This platform is ${process.platform}`);
```

The value `'android'` may also be returned if the Node.js is built on the Android
operating system. However, Android support in Node.js is experimental at this time.
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking nit: remove "at this time"

Trott
Trott approved these changes Jan 11, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with or without the nit/suggestion

gibfahn
gibfahn reviewed Jan 11, 2018
View reviewed changes
doc/api/process.md

```js
console.log(`This platform is ${process.platform}`);
```

The value `'android'` may also be returned if the Node.js is built on the
Android operating system. However, Android support in Node.js is experimental.
Copy link
Member

@gibfahn gibfahn Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@marswong , in reply to #18057 (comment)

Yes, I was replying to @Fishrock123 's suggestion that we link to the supported platforms table.

Maybe something like this:

Android support in Node.js is experimental.

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018
@joyeecheung
Copy link
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/97/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 19, 2018
@marswong @BridgeAR
doc: improve process.platform
36b13eb 
PR-URL: nodejs#18057
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in 36b13eb. Congratulations on your first commit to Node.js @marswong! 🎉

(Note: I actually added the suggestion to the commit while landing)

@BridgeAR BridgeAR closed this Jan 19, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@marswong @evanlucas
doc: improve process.platform
ff6e73a 
PR-URL: #18057
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@marswong @evanlucas
doc: improve process.platform
5c5aa49 
PR-URL: #18057
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@marswong @MylesBorins
doc: improve process.platform
bda8355 
PR-URL: #18057
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@marswong @MayaLekova
doc: improve process.platform
91fcc4c 
PR-URL: nodejs#18057
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn left review comments

@Fishrock123 Fishrock123 Fishrock123 left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.