Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distance_of_time_in_words method accepts args in both directions #3530

Closed
wants to merge 1 commit into from
Closed

distance_of_time_in_words method accepts args in both directions #3530

wants to merge 1 commit into from

Conversation

avakhov
Copy link
Contributor

@avakhov avakhov commented Nov 5, 2011

Method distance_of_time_in_words date_helper.rb#L70 was designed to be symmetrical for arguments swapping. It should also work well for supporting time_ago_in_words date_helper.rb#L132

ae7d0d8 (Take leap years into account more seriously ...) added unsymmetry for long time periods. I propose to fix it.

PS. The condtion with leap years calculation (leap_years = (fyear > tyear) ? 0 : ...) is reasonable. Although from_time will be always less or equal than to_time. fyear can be greater than tyear for example at March 1st.

@sarahhodne
Copy link
Contributor

What is the difference to doing it this way?

The tests pass for ruby 1.9.3-p0.

@avakhov
Copy link
Contributor Author

avakhov commented Nov 5, 2011

It allows to calc leap years correct: date_helper.rb#L98-102

@sarahhodne
Copy link
Contributor

I see. Could you throw in a test to check if leap years are calculated correctly as well, so we don't get this issue again later?

@avakhov
Copy link
Contributor Author

avakhov commented Nov 6, 2011

@dvyjones, I'm sorry explain please that test case did you mean. I tested just argument swapping wich I suggest in the request. Without fix assert R130 fails. distance_of_time_in_words(end_date, start_date) returns 'about 28 years' instead 'almost 28 years' as in previous assert.

@sarahhodne
Copy link
Contributor

@avakhov You said that the code didn't calculate leap years correctly. As far as I can see, we have no test to check if leap years are correctly calculated, so if you could add a test that basically does what you did when you found out that leap years aren't correctly calculated?

@avakhov
Copy link
Contributor Author

avakhov commented Nov 6, 2011

I see. I formulated the idea not quite exact.

Leap years are calculcated correct, but only if from_time > to_time. Method distance_of_time_in_words should work if from_time > to_time and if to_time < from_time (it was true before ae7d0d8 and it's declared in documentation for method). Without my fix the condition (fyear > tyear) is true in case of from_time > to_time and false for back order of args. In the second case leap years discount will be ignored.

I want to return symmetry to this method back.

@avakhov avakhov closed this Jan 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants