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 os.uptime() and process.uptime() descriptions #12294

Closed
wants to merge 1 commit into from
Closed

doc: update os.uptime() and process.uptime() descriptions #12294

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
Affected core subsystem(s)

doc, os

Fixes: #12291

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. os Issues and PRs related to the os subsystem. labels Apr 9, 2017
doc/api/os.md Outdated
*Note*: Within Node.js' internals, this number is represented as a `double`.
However, fractional seconds are not returned and the value can typically be
treated as an integer.
*Note*: the return value includes fractions of a second on Windows.
Copy link
Contributor

@refack refack Apr 9, 2017

Choose a reason for hiding this comment

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

I would phrase it as:
-*Note*: On Windows (and maybe other platforms as well) the value returned includes fractions of a second.

Copy link
Member

@gibfahn gibfahn Apr 9, 2017

Choose a reason for hiding this comment

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

(and maybe other platforms as well)

The docs are the canonical source for what our API does, we should be exact. I don't think there are any other platforms that do fractions of a second, but if there are, we can update the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should be exact.

@gibfahn I agree in principle, can we figure this out exactly? Until recently we had a hard time list all supported platform (#11872). Mentioning Windows has an implicit meaning that it's only on Windows. And as @bnoordhuis said

UNIX implementations of that function don't currently bother to mix in the sub-second fraction. That could change in the future though, if someone requests it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So:
-*Note*: The value returned may include fractions of a second (on Windows it consistently does, on others it's not defined).

Copy link
Member

@gibfahn gibfahn Apr 9, 2017

Choose a reason for hiding this comment

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

I checked macOS, Ubuntu 16.04, SmartOS 16, FreeBSD 11, and AIX 6.1, and they're all consistent (os.uptime() is an integer, process.uptime() is not). Given that, I'd say we're okay to specify that it's just Windows.

If it changes in the future then the PR to change it should update the docs, making it a semver-major change.

Copy link
Contributor

@refack refack Apr 9, 2017

Choose a reason for hiding this comment

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

-*Note*: The value returned may include fractions of a second (on Windows it consistently returns a float. Other platforms currently returns an integer but may change in the future).

Copy link
Contributor

Choose a reason for hiding this comment

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

PR to change it should update the docs, making it a semver-major change.

It might get changed by the OS, under us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack Could you repeat the proposed wording in the main thread? Somebody may not look in the outdated discussion.

@refack
Copy link
Contributor

refack commented Apr 9, 2017

maybe add the same to process.md:1669

@vsemozhetbyt
Copy link
Contributor Author

@refack process.uptime() doc does not state to return integer and it is the same for all OS, it seems.

@gibfahn
Copy link
Member

gibfahn commented Apr 9, 2017

maybe add the same to process.md:1669

@vsemozhetbyt You could just include this bit in process.md:

Note: the return value includes fractions of a second. Use Math.floor() to get whole seconds.

@vsemozhetbyt
Copy link
Contributor Author

Is (and maybe some other platforms as well) OK?

@refack
Copy link
Contributor

refack commented Apr 9, 2017

@vsemozhetbyt No. I've rephrased, and would wait for a +1 from some else
/cc @gibfahn @bnoordhuis

@refack
Copy link
Contributor

refack commented Apr 9, 2017

gibfahn 8 minutes ago • edited Member
I checked macOS, Ubuntu 16.04, SmartOS 16, FreeBSD 11, and AIX 6.1, and they're all consistent (os.uptime() is an integer, process.uptime() is not). Given that, I'd say we're okay to specify that it's just Windows.

If it changes in the future then the PR to change it should update the docs, making it a semver-major change.

refack 5 minutes ago • edited Member
-Note: The value returned may include fractions of a second (on Windows it consistently returns a float. Other platforms currently returns an integer but may change in the future).

refack 4 minutes ago Member
PR to change it should update the docs, making it a semver-major change.
It might get changed by the OS, under us

Proposing:
-*Note*: The value returned may include fractions of a second (Windows consistently returns a float. Other platforms currently return a whole number, but may change this behaviour in the future).

@refack
Copy link
Contributor

refack commented Apr 9, 2017

I have too many lawyer friends...

@gibfahn
Copy link
Member

gibfahn commented Apr 9, 2017

It might get changed by the OS, under us

I'm pretty sure the UNIX implementations @bnoordhuis is talking about in #12291 (comment) are in libuv, not in the OS itself. So it can only change in a libuv update (which would need to update the docs).

EDIT: e.g. here

See also libuv/libuv@556fe1a

@refack
Copy link
Contributor

refack commented Apr 9, 2017

are in libuv, not in the OS itself

Let me look...

@refack
Copy link
Contributor

refack commented Apr 9, 2017

are in libuv, not in the OS itself

yep, saw it.

So phrase:
*Note*: On Windows the returned value includes fractions of a second...

@refack
Copy link
Contributor

refack commented Apr 9, 2017

Ref: libuv/libuv#1294

@vsemozhetbyt vsemozhetbyt changed the title doc: fix os.uptime() description in os.md doc: update os.uptime() and process.uptime() descriptions Apr 10, 2017
@vsemozhetbyt
Copy link
Contributor Author

I've updated according to the last proposition for now.

@nodejs/documentation PTAL

@vsemozhetbyt vsemozhetbyt added libuv Issues and PRs related to the libuv dependency or the uv binding. lts-watch-v4.x windows Issues and PRs related to the Windows platform. labels Apr 10, 2017
jasnell pushed a commit that referenced this pull request Apr 11, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 11, 2017

Landed in 9b6376a

@jasnell jasnell closed this Apr 11, 2017
@vsemozhetbyt vsemozhetbyt deleted the os-uptime branch April 11, 2017 09:39
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

Can someone confirm that that is true for v6.x

@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Can someone confirm that that is true for v6.x

ping @vsemozhetbyt

@vsemozhetbyt
Copy link
Contributor Author

@gibfahn It seems this is true for the last v6. Does this land cleanly or should I make a backport PR?

gibfahn pushed a commit that referenced this pull request Jun 18, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Lands cleanly

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #12294
Fixes: #12291
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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. libuv Issues and PRs related to the libuv dependency or the uv binding. os Issues and PRs related to the os subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants