console: update time formatting #29629
Conversation
nodejs-github-bot
added
the
console
I think this can be a reasonable choice for end-user-facing output, but generally isn’t for developer-facing output (like the console time APIs) where easy comparisons are more important than having an easy time converting the value to how it would be described in informal conversation. If you do want to display these values separately, I’d suggest using a more standard format like |
This improves the readability of the `console.timeEnd()` output while keeping a higher output's precision in multiple cases. Instead of e.g. '1.005min' it will print '1m and 300ms'.
BridgeAR
force-pushed
the
format-time
branch
from
9058d7d
to
1c5e1b1
Compare
|
@addaleax I mainly switched to your suggestion. PTAL |
Trott
added
the
author ready
This comment has been minimized.
This comment has been minimized.
|
@nodejs/console |
| assert.strictEqual(formatTime(60300.3), '1:00.300 (m:ss.mmm)'); | ||
| assert.strictEqual(formatTime(4000457.4), '1:06:40.457 (h:mm:ss.mmm)'); | ||
| assert.strictEqual(formatTime(3601310.4), '1:00:01.310 (h:mm:ss.mmm)'); | ||
| assert.strictEqual(formatTime(3213601017.6), '892:40:01.018 (h:mm:ss.mmm)'); |
There was a problem hiding this comment.
so we're outputting the literal string "hh:mm:as.mmm"? i think this format is standard enough that we don't have to do that.
There was a problem hiding this comment.
I guess most people will know about it but I added it to leave no doubt. I would like to support people who are not yet that experienced as well.
| assert.strictEqual(formatTime(100.0096), '100.01ms'); | ||
| assert.strictEqual(formatTime(100.0115), '100.011ms'); | ||
| assert.strictEqual(formatTime(1500.04), '1.500s'); | ||
| assert.strictEqual(formatTime(1000.056), '1.000s'); |
There was a problem hiding this comment.
I would keep these consistent with the higher numbers, to be honest...
e.g.
| assert.strictEqual(formatTime(1000.056), '1.000s'); | |
| assert.strictEqual(formatTime(1000.056), '1.000 (ss.mmm)'); |
There was a problem hiding this comment.
Seconds and milliseconds are the most common ones. People will rarely reach the higher values and it seems more straight forward to me to keep s and ms as common cases.
There was a problem hiding this comment.
Yeah I understand the reasoning, I just disagree :) I'd rather opt for consistency.
There was a problem hiding this comment.
I'm going to remove the author ready label out of caution while this gets sorted, but feel free to re-add it if that's overly-cautious of me.
There was a problem hiding this comment.
@jasnell is your comment blocking? I would rather close this PR than changing the common cases (even though I believe that this PR does improve the overall output).
There was a problem hiding this comment.
Not blocking but I definitely don't consider the inconsistency to be ideal.
Trott
removed
the
author ready
This improves the readability of the `console.timeEnd()` output while keeping a higher output's precision in multiple cases. Instead of e.g. '1.005min' it will print '1:00.300 (m:ss.mmm)'. PR-URL: #29629 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
|
Landed in 204248a |
This improves the readability of the
console.timeEnd()outputwhile keeping a higher output's precision in multiple cases.
Instead of e.g. '1.005min' it will print '1min and 300ms'.
It also rounds full numbers (e.g.,
1.1instead of1.100). It would also be possible to always print seconds and milliseconds as one value, since that's easy to read. This just seemed even clearer and more straight forward.This relies on a semver-major commit (#29251). As long as it lands together with the other commit, it should be fine to land this as a patch instead of being semver-major itself.
I requested the reviewers from the former commit to also review this PR.
CC @Xstoudi
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes