Do you have a test case that fails?
Actually, you're right -- it doesn't fail with Time.local instances.
But it does fail with TimeWithZone instances, as of of this commit:
. There's an open ticket on Lighthouse to add this same behavior to
Time.local instances: http://rails.lighthouseapp.com/projects/8994/tickets/504-rails-2-1-dst-issues
But this isn't directly relevant to you patch, other than, when this
Time.local bug (a holdover from pre-Duration days) is fixed,
Time.local(2008, 11, 2) + 1.5.hours + 1.5.hours will (correctly) no
longer be equal to Time.local(2008, 11, 2) + 3.hours.
I think Koz is right, better to have this somewhat counter-intuitive
edge case than the current behavior of ignoring the fractional part
of :days and :weeks entirely.
Given that we're fixing :days and :weeks options, would it make sense
to fix :months and :years args as well?
This is where it starts to get iffy... (normal?) people almost always
expect a week to be 7 days, and a day to be 24 hours, but they accept
that a month is not a fixed duration.
We could have years behave in the same way, so 0.5 years ago is 6
months ago, so we can just have:
unless options[: years].nil?
options[:years], partial_ years = options[: years].divmod(1)
options[: months] = (options[:months] || 0) + 12 *
at the top, and that would be fine.... apart from a decimal number of
months does not work.... so where do we go from here:
unless options[: months].nil?
options[: months], partial_months = options[:
options[: days] = (options[:days] || 0) + ??? *
so what goes where the ??? is now the question?
Do we try and wind the date forward by the known years, months and
days we have, then find the number of days between that date and the
same date next month?
Or do we assume 30 days and hope no one notices ?
Basically partial months are icky, and partial years depend on them,
unless we want to just assume a year is 365 days? But it feels more
natural to go the Years => Months => Days => H/M/S route (just don't
think about weeks).
We could also consider 52 weeks in a year as an option? But this still
skips out months...
The last option, is removing support for fractional months (and
years?) by making Numeric#months a method on Integer instead of
Numeric? We could even put a depreciation error message on
Which way are other people thinking?