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: Update docs for os.platform() #2446

Closed
wants to merge 1 commit into from
Closed

doc: Update docs for os.platform() #2446

wants to merge 1 commit into from

Conversation

hertzg
Copy link

@hertzg hertzg commented Aug 19, 2015

Specifies origin and includes a list of possible values

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

/ping @jasnell

@brendanashworth brendanashworth added doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem. labels Aug 19, 2015
, but with exceptions in case of Mac OS X, Windows and Solaris.

On Windows its value is always `"win32"`, on Mac OS X its `"darwin"` and on
Solaris its `"sunos"`.
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

hertzg commented Aug 19, 2015

@targos Fixed all, thanks

@targos
Copy link
Member

targos commented Aug 20, 2015

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.

On Windows its value is always `"win32"`, on Mac OS X it's `"darwin"` and on
Solaris it's `"sunos"`.

Here's a list of possible values for some 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

hertzg commented Aug 22, 2015

@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

jasnell commented Nov 5, 2015

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

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

Closing because of inactivity.

@silverwind silverwind closed this Feb 28, 2016
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

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 closed nodejs#2446
* updates broken documentation link
* fixes copy based on comments from @sam-github
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. os Issues and PRs related to the os subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants