Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

DateTime#advance fails to account for partial days #12005

Closed
lukfugl opened this Issue · 13 comments

6 participants

Jacob Fugal Robin Dupret Andrew Hooker Rafael Mendonça França Ruby on Rails Bot Abdelkader Boudih
Jacob Fugal

some_date_time.advance(:days => 1.5) will return a DateTime exactly 1 day after some_date_time, not 1.5 days.

Compare to Time#advance. some_time.advance(:days => 1.5) catches the 0.5 as 12 hours (while not necessarily 100% accurate around DST boundaries, it's close enough) and returns a Time 1 day and 12 hours (not necessarily 36 hours, because DST) after some_time. This is the behavior I'd also expect from DateTime#advance.

This trickles down to affect some_date_time + 1.5.days as well.

I think it would be sufficient to carry over to partial day and partial week logic from Time#advance into DateTime#advance. Or better, refactor #advance out in a different module with the correct logic and have both Time and DateTime include it. Suprisingly, I found 3 unique "refactor the time stuffs" pull requests, yet none of them noticed or addressed this issue.

Robin Dupret
Collaborator

@lukfugl : Which version of Rails are you using please? I can't reproduce it on 4.0.0:

Loading development environment (Rails 4.0.0)
>> t = 2.days.ago
=> Thu, 22 Aug 2013 09:24:08 UTC +00:00
>> t.advance(days: 1.5)
=> Fri, 23 Aug 2013 21:24:08 UTC +00:00
Andrew Hooker

i can't reproduce on 3.2.14 either.

irb(main):001:0>  t = 2.days.ago
=> Fri, 23 Aug 2013 21:17:32 UTC +00:00
irb(main):002:0>  t.advance(days: 1.5)
=> Sun, 25 Aug 2013 09:17:32 UTC +00:00
Jacob Fugal

That's because it works fine for Time (and by extension ActiveSupport::TimeWithZone) as I already mentioned in the report. 2.days.ago gives you an ActiveSupport::TimeWithZone object, not a DateTime object.

Here's an example of the behavior with DateTime:

Loading development environment (Rails 4.0.0)
irb(main):001:0> 2.days.ago.class
=> ActiveSupport::TimeWithZone
irb(main):002:0> 2.days.ago.to_datetime.class
=> DateTime
irb(main):003:0> 2.days.ago.to_datetime
=> Sun, 25 Aug 2013 16:33:40 +0000
irb(main):004:0> 2.days.ago.to_datetime.advance(days: 1.5)
=> Mon, 26 Aug 2013 16:33:45 +0000
aviaron aviaron referenced this issue from a commit in n0nick/rails
Avia Aharon Add tests for DateTime partial day advance (bug #12005) ca241fc
Jacob Fugal lukfugl added the stale label
Rafael Mendonça França
Owner

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Jacob Fugal

I upgraded my rails gem (to 4.1.1) just now and performed the duplication from my above comment:

Loading development environment (Rails 4.1.1)
irb(main):001:0> value = 2.days.ago.to_datetime
=> Wed, 14 May 2014 00:09:12 +0000
irb(main):002:0> value.advance(days: 1.5)
=> Thu, 15 May 2014 00:09:12 +0000
# expected: Thu, 15 May 2014 12:09:12

If necessary, I'll update my Gemfile to reference a specific github branch and duplicate, but I'm pretty sure it's still there.

Ruby on Rails Bot
Collaborator

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Ruby on Rails Bot rails-bot closed this
Jacob Fugal

Seriously? I just commented two weeks ago with the requested proof that the bug is still present. That's inactivity?

Rafael Mendonça França

Opps, sorry, for some reason missed your comment. Reopened. Thanks.

Rafael Mendonça França rafaelfranca reopened this
Rafael Mendonça França rafaelfranca removed the stale label
Abdelkader Boudih
Collaborator

value.advance(days: 1, hours: 12) as workaround for now.

Jacob Fugal

Thanks @rafaelfranca. Also, sorry about the tone of my earlier comment; I was caught off guard by the closure and reacted poorly.

@seuros yeah, agreed that does work and is a good workaround. I just see an opportunity for DateTime to be consistent with its kin, so didn't want the issue disappearing from the tracker. :)

Rafael Mendonça França

No problem. I had to review every stale issue individually and sometimes I can miss something, so it is fine to ask to reopen if it was closed by mistake.

I saw there is a PR attached to this issue so I added the attached PR tag to not mark this issue as stale again.

Rafael Mendonça França

Closed by #12080

Jacob Fugal

Sweet. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.