EvanHahn / HumanizeDuration.js

361000 becomes "6 minutes, 1 second"
https://evanhahn.github.io/HumanizeDuration.js/
The Unlicense
1.64k stars 175 forks source link

fix: display decimals on largest unit #171

Closed HHK1 closed 3 years ago

HHK1 commented 4 years ago

Fixes https://github.com/EvanHahn/HumanizeDuration.js/issues/150

As explained in the issue, using the library with largest set (with or without units specified), results in leaving the decimal part for units larger than second.

Now, some tests are breaking, specifically:

const h = humanizer({ largest: 2 })
assert.strictEqual(h(540360012), '6 days, 6 hours')
assert.strictEqual(h(540360012, { largest: 3 }), '6 days, 6 hours, 6 minutes')

I haven't touched those to avoid hiding this. I think it's reasonable to have those tests failed if round or maxDecimalPoints is not provided, the only issue is that it's going to be a "breaking" change in those cases.

You can leave it as is (and fix the test ofc), and publish a major version, or only apply the remainder if maxDecimalPoints has been provided, what do you think ?

EvanHahn commented 4 years ago

Thanks for this.

I won't have a chance to look at this for a few days, but I've written down to take a look soon.

EvanHahn commented 4 years ago

Thanks for doing this. I think this makes sense to do as a breaking change, so would you mind fixing the broken tests?

HHK1 commented 4 years ago

Fixed the tests in https://github.com/EvanHahn/HumanizeDuration.js/pull/171/commits/53f05719470029d9b1be3fee097758c02b45fd61

I also realised that passing Number.Infinity was now causing a crash, so I added some tests and fixed it. Behaviour is the same as before, and is now tested: https://github.com/EvanHahn/HumanizeDuration.js/pull/171/commits/8433326dc8a74461306a6eab5979fa3df2633867

EvanHahn commented 4 years ago

Thanks. I plan to take another look in the next week or so.

HHK1 commented 4 years ago

Hey @EvanHahn , any luck of having this part of the next release ? Is there anything I can do to help you review this ?

EvanHahn commented 4 years ago

Sorry that I've been slow to respond here. I'm trying to take a more holistic look at the library to address some bugs like this (among others).

HHK1 commented 4 years ago

Hi @EvanHahn, just checking if you want me to update this, maybe target your v4.0.0 branch directly instead of master ?

EvanHahn commented 4 years ago

Sorry for moving slowly here. I'll get back to you.

EvanHahn commented 3 years ago

I just renamed master to main which caused this PR to be automatically closed. I didn't realize this would happen. Apologies!