Fix for issue #3560 #5181

Merged
merged 2 commits into from Apr 30, 2012

Conversation

Projects
None yet
3 participants
@mcmire
Contributor

mcmire commented Feb 26, 2012

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).

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 30, 2012

Owner

@jeremy we can merge this one now. All the test green and this is the desirable behavior.

Owner

rafaelfranca commented Apr 30, 2012

@jeremy we can merge this one now. All the test green and this is the desirable behavior.

@jeremy

View changes

actionpack/test/template/date_helper_test.rb
- # 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

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Apr 30, 2012

Owner

I don't understand this comment.

@jeremy

jeremy Apr 30, 2012

Owner

I don't understand this comment.

This comment has been minimized.

Show comment Hide comment
@mcmire

mcmire Apr 30, 2012

Contributor

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.

@mcmire

mcmire Apr 30, 2012

Contributor

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.

This comment has been minimized.

Show comment Hide comment
@mcmire

mcmire Apr 30, 2012

Contributor

Okay, pull request updated.

@mcmire

mcmire Apr 30, 2012

Contributor

Okay, pull request updated.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 30, 2012

Owner

@mcmire we need to rebase it again. A lot of thing changed in these helpers.

Owner

rafaelfranca commented Apr 30, 2012

@mcmire we need to rebase it again. A lot of thing changed in these helpers.

@mcmire

This comment has been minimized.

Show comment Hide comment
@mcmire

mcmire Apr 30, 2012

Contributor

Rebased. I also took the opportunity to redo the commits.

Contributor

mcmire commented Apr 30, 2012

Rebased. I also took the opportunity to redo the commits.

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Apr 30, 2012

Owner

Thanks for explaining, @mcmire ! That makes sense.

Owner

jeremy commented Apr 30, 2012

Thanks for explaining, @mcmire ! That makes sense.

jeremy added a commit that referenced this pull request Apr 30, 2012

Merge pull request #5181 from mcmire/issue_3560
Handle distance_of_time_in_words boundary conditions. Fixes #3560.

@jeremy jeremy merged commit b7ec875 into rails:master Apr 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment