Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

distance_of_time_in_words calculates wrong no of years #410

Merged
merged 2 commits into from May 6, 2011

Conversation

Projects
None yet
2 participants
Contributor

asanghi commented May 6, 2011

distance_of_time_in_words does not take leap years into account accurately. The bug was raised on Lighthouse 6074.

Patch with test provided. Feel free to comment/review/suggest changes.

Aditya Sanghi added some commits Nov 29, 2010

Contributor

josevalim commented May 6, 2011

Can we measure how much this new implementation will affect performance? In general, the Date class is not very fast.

Contributor

asanghi commented May 6, 2011

Having a quick look at Date.leap? reveals that it instantiates 1st March on the year and checks if the previous day was a 29th? This implementation would do it for each year given in the range.

How much of a performance hit can we bargain for accuracy? :)

Will measure and post gist.

Contributor

josevalim commented May 6, 2011

I am worried how this will affect common cases like minutes, hours and 1 to 3 years. Maybe we could have a short circuit logic that simply skip this calculus if < 2 years. If you are working with old dates, this accuracy is indeed needed.

Contributor

asanghi commented May 6, 2011

@josevalim shorter distances of time are handled in the distance_of_time_in_words method already to some extent.

I think 525599 is the number of minutes in 365 days, so upto 1 year is already handled.

The whole method seems prime for refactoring but will now concoct to handle for < 2 years.

Contributor

josevalim commented May 6, 2011

Ah, great, no performance worries then! I am merging this in. A refactoring is indeed welcome, at least the leap year part.

@josevalim josevalim added a commit that referenced this pull request May 6, 2011

@josevalim josevalim Merge pull request #410 from asanghi/LH6074
distance_of_time_in_words calculates wrong no of years
be9857c

@josevalim josevalim merged commit be9857c into rails:master May 6, 2011

@jake3030 jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011

@fcheung @jeremy fcheung + jeremy Make constantize look into ancestors
[#410 state:resolved]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
262fef7

@jake3030 jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011

@jeremy jeremy Revert "Make constantize look into ancestors"
[#410 state:open]

This reverts commit 262fef7.
19be3d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment