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

Properly add duration parts #6581

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

zmack commented Jun 1, 2012

Fixes the issues reported by #6559

Owner

pixeltrix commented Jun 1, 2012

Superficially, this appears to be okay but we need to give it some thought. This is the behaviour now:

>> Date.civil(2012, 1, 31) + 1.month + 1.month
=> Thu, 29 Mar 2012
>> Date.civil(2012, 1, 31) + (1.month + 1.month)
=> Thu, 29 Mar 2012

and after applying this PR:

>> Date.civil(2012, 1, 31) + 1.month + 1.month
=> Thu, 29 Mar 2012
>> Date.civil(2012, 1, 31) + (1.month + 1.month)
=> Thu, 31 Mar 2012

Seems slightly counter-intuitive to me.

Contributor

pokonski commented Jun 1, 2012

At first it did seem counter-intuitive, but it actually makes sense to me. This gives a choice to the programmer whether he wants to increment months for example in a loop (month days are 'truncated' in every interation), or in one step (grouped in parentheses).

Owner

pixeltrix commented Jun 1, 2012

@pokonski I agree that maybe desired but it should be explicit method e.g.:

>> Date.civil(2012, 1, 31) + (1.month + 1.month).flatten # or compact, or whatever
=> Thu, 31 Mar 2012
Contributor

pokonski commented Jun 1, 2012

@pixeltrix That's even better!

@ghost ghost assigned pixeltrix Jun 1, 2012

Contributor

zmack commented Jun 5, 2012

Closing this as the correct approach to the bug referenced would be to just make equality parts-aware.

Thanks for everyone who pointed that out.

@zmack zmack closed this Jun 5, 2012

Contributor

mcmire commented Jun 30, 2012

I actually think this is a fine pull request. As @pixeltrix says, if you add a month to a date, and then add another month, there is going to be separate behavior as opposed to adding 2 months all at once. That's a fact. I think the two cases that were brought up -- + 1.month + 1.month vs. + 2.months -- are kind of moot. Dates are hard. If you have a loop and are starting from a date and incrementing it by a month every iteration, you're going to run into this issue anyway. If you're doing something like date + 1.month + some.months you should expect that it's going to execute date + 1.month before executing + some.months. It doesn't read well, but that's what you get for trying to make your Ruby read like English :trollface:

Contributor

iwiznia commented Oct 8, 2013

Arriving a year late to the conversation, but maybe I can bring something to the table...
I understand why advancing 1 month and then 1 month again truncates the days, but it's still counter intuitive if your are making operations with the durations... a better example would be:

duration = (1.month - 1.month)  # returns 0, but internally it has 2 parts, month 1 and month -1. 
Date.new(2013,1,31) + duration # returns the same as Date.new(2013,2,28)

I think the best solution would be:

  • if you add a duration to a date, the current behaviour applies.
  • if you add 2 durations, they should be "flattened" as @pixeltrix said.

Examples:

(Date.new(2013,1,31) + 1.month) + 1.month # would return Date.new(2013,3,28)

Date.new(2013,1,31) + (1.month + 1.month) # would return Date.new(2013,3,31)

Date.new(2013,1,31) + 2.months # would return Date.new(2013,3,31) the same as the one right above this, which what's expected, than 1.month + 1.month === 2.months

What do you think???

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