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

Add colonNotation option #37

Merged
merged 12 commits into from
Nov 22, 2019
Merged

Add colonNotation option #37

merged 12 commits into from
Nov 22, 2019

Conversation

villebro
Copy link
Contributor

This PR adds option to format using colon notation. Examples from tests.js:

prettyMilliseconds(1000, {colonNotation: true}) //=> '0:01'
prettyMilliseconds(1500, {colonNotation: true}) //=> '0:01.5'
prettyMilliseconds(1543, {colonNotation: true, secondsDecimalDigits: 3}) //=> '0:01.543'
prettyMilliseconds(1000 * 60, {colonNotation: true}) //=> '1:00'
prettyMilliseconds(1000 * 90, {colonNotation: true}) //=> '1:30'
prettyMilliseconds(95500, {colonNotation: true}) //=> '1:35.5'
prettyMilliseconds((1000 * 60 * 59) + (1000 * 59) + (500), {colonNotation: true}) //=> '59:59.5'

sindresorhus
sindresorhus requested changes Oct 3, 2019
View reviewed changes
test.js Outdated

test('work with colon output option', t => {
t.is(prettyMilliseconds(1000, {colonNotation: true}), '0:01');
t.is(prettyMilliseconds(1500, {colonNotation: true}), '0:01.5');
Copy link
Owner

Choose a reason for hiding this comment

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

This does not match what the readme says, which is 5:01:45. Why wouldn't seconds be in the same format as minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hours, minutes and seconds are prefixed with :, and anything below seconds are treated with regular decimals. So 5h 1m 45s -> ´5:01:45´, but 5h 1m 45.5s -> 5:01:45.5 etc. See the comment below for more context.

test.js
@@ -221,3 +221,15 @@ test('properly rounds milliseconds with secondsDecimalDigits', t => {
t.is(fn((3600 * 1e3) - 1), '1 hour');
t.is(fn((2 * 3600 * 1e3) - 1), '2 hours');
});

test('work with colon output option', t => {
t.is(prettyMilliseconds(1000, {colonNotation: true}), '0:01');
Copy link
Owner

Choose a reason for hiding this comment

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

Why is minute zero prefixed, but hour is not?

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 tried googling for an official standard outlining how this format should behave, but was unfortunately unable to find one. However, the attached screenshot from the Kona Ironman World Championships this year shows what the commonly accepted convention is:
image
Here we can see, that Jan Frodeno's total time 2h 24m 45s was 1s behind whoever was leading at that time, which in this notation would be expressed as 2:24:45 and 0:01 respectively. This example doesn't show fractional seconds, but for seconds regular decimal formatting applies.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with your assessment.

test.js Outdated
t.is(prettyMilliseconds(1000 * 90, {colonNotation: true}), '1:30');
t.is(prettyMilliseconds(95500, {colonNotation: true}), '1:35.5');
t.is(prettyMilliseconds(95511.111, {colonNotation: true, formatSubMilliseconds: true}), '1:35.5');
t.is(prettyMilliseconds((1000 * 60 * 59) + (1000 * 59) + (500), {colonNotation: true}), '59:59.5');
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to see more tests, with this option combined with all the other options.

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 added a bunch of tests which actually revealed a few bugs in my code. I also made the code slightly more verbose, as I found it started becoming difficult to follow with all the conditionalities. I hope the refactored code is more legible.

index.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add colonNotation Add colonNotation option Oct 3, 2019
@villebro
Copy link
Contributor Author

villebro commented Nov 5, 2019

Thanks for the review @sindresorhus , I'll address your comments shortly.

sindresorhus
sindresorhus reviewed Nov 16, 2019
View reviewed changes
readme.md Outdated
@@ -104,19 +111,30 @@ Default: `false`

Use full-length units: `5h 1m 45s` → `5 hours 1 minute 45 seconds`

Setting `colonNotation` to `true` overrides this option.
Copy link
Owner

Choose a reason for hiding this comment

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

Same as https://github.com/sindresorhus/pretty-ms/pull/37/files#r347084918

sindresorhus
sindresorhus reviewed Nov 16, 2019
View reviewed changes
readme.md Outdated
##### separateMilliseconds

Type: `boolean`\
Default: `false`

Show milliseconds separately. This means they won't be included in the decimal part of the seconds.

Setting `colonNotation` to `true` overrides this option.
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of writing this for each option, I think it would be better to just document in the colonNotation docs what options it overrides.

sindresorhus
sindresorhus reviewed Nov 16, 2019
View reviewed changes
readme.md Outdated
Type: `boolean`<br>
Default: `false`

Use colon notation: `5h 1m 45s` → `5:01:45`
Copy link
Owner

Choose a reason for hiding this comment

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

The description should be the same as in index.d.ts

sindresorhus
sindresorhus reviewed Nov 16, 2019
View reviewed changes
readme.md
- `compact`
- `formatSubMilliseconds`
- `separateMilliseconds`
- `verbose`
Copy link
Owner

Choose a reason for hiding this comment

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

Remember to keep index.d.ts in sync.

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 noticed some of the others were slightly out of sync (more comprehensive in readme), ok if I sync those at the same time?

Copy link
Owner

Choose a reason for hiding this comment

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

👍

@villebro
Copy link
Contributor Author

@sindresorhus I believe this is now ready; will be happy to make any additional changes if needed.

@sindresorhus sindresorhus merged commit b672415 into sindresorhus:master Nov 22, 2019
@villebro villebro deleted the colon branch November 22, 2019 06:41
@villebro villebro mentioned this pull request Nov 22, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Mar 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@villebro @sindresorhus