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 Bug with mode {round = true} #101

Closed ntanitime closed 8 years ago

ntanitime commented 8 years ago

In case like this:

let time = humanizeDuration(5219998, {
            delimiter: ' and ',
            units: ['d', 'h', 'm'],
            round = true,
            largest: 2
});
console.log(time);

The old algorithm give a wrong result: "1 hour". With my fix : "1 hour and 27 minutes"

EvanHahn commented 8 years ago

It looks like there was a linting error:

humanize-duration.js:362:9: "firstOccupiedUnitIndex" is defined but never used

Could you take a look at this and update this PR?

ntanitime commented 8 years ago

Done!

EvanHahn commented 8 years ago

It looks like a couple of other tests are failing.

You can try pushing new commits to run these tests but running npm test is probably best.

ntanitime commented 8 years ago

Sorry, It's my first pull request! Now my fix should be fine :)

ntanitime commented 8 years ago

Done!

EvanHahn commented 8 years ago

I've been busy—I'll take a look at this soon.

EvanHahn commented 8 years ago

Finally took a look at this—so sorry it took so long.

The current behavior actually looks correct to me:

humanizeDuration(5219998, {
  delimiter: ' and ',
  units: ['d', 'h', 'm'],
  round: true,
  largest: 2
})

// => '1 hour and 27 minutes'

Are you getting different results?

EvanHahn commented 8 years ago

@ntanitime Any thoughts?