Skip to content

Conversation

@hertzg
Copy link

Specifies origin and includes a list of possible values

Ref: nodejs/node-v0.x-archive#25777

/ping @jasnell

@brendanashworthbrendanashworth added doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem. labels Aug 19, 2015
Copy link
Member

Choose a reason for hiding this comment

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

... on Mac OS X it is (or it's) ... on Solaris it is ...

Specifies origin and includes a list of possible values
@hertzg
Copy link
Author

@targos Fixed all, thanks

@targos
Copy link
Member

LGTM

@silverwind
Copy link
Contributor

Why do we even have os.platform() when it just returns process.platform? Legacy reasons?

Regarding this PR: I feel the changes are a bit too verbose, and I'm not sure the background on how this variable is obtained is really relevant to the user. Also, please update process.platform with the new platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, this feels to verbose. How about changing this whole part to:

Returns the value of `process.platform`, Possible values are: 

Also, please linkify process.platform.

Copy link
Author

Choose a reason for hiding this comment

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

As its values is assigned on build and not during runtime. I believe its required to show the origin of it.

Totally agree on linking,

@sam-github
Copy link
Contributor

I agree with @silverwind, its too verbose, has information that's not useful, and also makes the docs worse in a way, because it causes the os.platform and process.platform docs to diverge.

Better to simplify the os.platform() docs, link to process.platform, and then enumerate more platform values there.

And about "Linux & Others"... what other non-linux platforms are identified as "linux"?

@Qard
Copy link
Member

Qard commented Aug 20, 2015

👍 to putting platform map in process.platform list and just linking the os.platform() docs to it.

@hertzg
Copy link
Author

@silverwind@sam-github
Thanks for the comments, replied to all of them.

This was discussed firstly on joyent/node, please see the references.

nodejs/node-v0.x-archive#25769
nodejs/node-v0.x-archive#25777

@sam-github
Copy link
Contributor

@hertzg you replied to all comments, but didn't push new commits to address the comments, I've not much to offer until then.

@jasnell
Copy link
Member

@hertzg ... ping. did you see @sam-github 's last comment (#2446 (comment))

@jasnelljasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@silverwind
Copy link
Contributor

Closing because of inactivity.

@jasnell
Copy link
Member

That's unfortunate. This was a good change, just needed to make sure the details were correct. Hopefully someone will pick this up again.

markmachine pushed a commit to markmachine/node that referenced this pull request Dec 1, 2016
- made the copy changes @sam-github suggested - updated the documentation link as the original was broken references nodejs#2446
markmachine pushed a commit to markmachine/node that referenced this pull request Dec 1, 2016
* references closednodejs#2446 * updates broken documentation link * fixes copy based on comments from @sam-github
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.osIssues and PRs related to the os subsystem.stalledIssues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@hertzg@targos@silverwind@sam-github@Qard@jasnell@brendanashworth