Fix for issue #3560 #5181
Fix for issue #3560 #5181
Conversation
|
@jeremy we can merge this one now. All the test green and this is the desirable behavior. |
| # First case 0..1 | ||
| # 30 seconds is the boundary here because when we take the seconds | ||
| # difference between the two times, we round to the nearest minute... | ||
| # so we are kind of ignoring that here |
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
You're right, it's foggy. What I was going after was that the different time intervals that distance_of_time_in_words handles (everything 2 minutes and after) -- there's an extra ~30 seconds around the interval which is actually included in the conversion. For instance, 90 minutes up to 24 hours is actually 89m30s up to 23h59m29s. The case statement doesn't include this information because distance_in_minutes is rounded at the beginning of the method. That's something we don't have to mention in the method itself because it's only valuable from the perspective of tests, but if you are reading these tests and you don't catch that the minutes are being rounded then you might wonder why a 30-second boundary is being tested here.
I can revise this comment to reflect this thinking. I also noticed the docs for distance_of_time_in_words needs to be revised as well, I'll do that shortly.
There was a problem hiding this comment.
Okay, pull request updated.
|
@mcmire we need to rebase it again. A lot of thing changed in these helpers. |
|
Rebased. I also took the opportunity to redo the commits. |
|
Thanks for explaining, @mcmire ! That makes sense. |
Handle distance_of_time_in_words boundary conditions. Fixes #3560.
The problem here was that converting days 45..60 would be converted to "about 1 month ago" instead of "2 months ago". This is because distance_of_time_in_words doesn't round the days to the nearest month (it does do this for > 60 days, but not before).