Allow Range including DateTime and DateTime::Infinity #8178

Closed
lgierth opened this Issue Nov 12, 2012 · 4 comments

Comments

Projects
None yet
3 participants

lgierth commented Nov 12, 2012

With activesupport-3.2.8, picking only the DateTime conversions:

1.9.3p327 :002 >   require 'active_support/core_ext/date_time/conversions'
 => true 
1.9.3p327 :003 > DateTime::Infinity.new..DateTime::Infinity.new
 => #<Date::Infinity:0x8465944 @d=1>..#<Date::Infinity:0x8465930 @d=1> 
1.9.3p327 :004 > DateTime.new..DateTime::Infinity.new
ArgumentError: bad value for range
    from (irb):4
    from /home/lars/.rvm/rubies/ruby-1.9.3-p327/bin/irb:16:in `<main>'

Expected:

1.9.3p327 :001 > require 'active_support'
 => true 
1.9.3p327 :002 > DateTime::Infinity.new..DateTime::Infinity.new
 => #<Date::Infinity:0x8e9d2e0 @d=1>..#<Date::Infinity:0x8e9d2b8 @d=1> 
1.9.3p327 :003 > DateTime.new..DateTime::Infinity.new
 => #<DateTime: -4712-01-01T00:00:00+00:00 ((0j,0s,0n),+0s,2299161j)>..#<Date::Infinity:0x90c8f88 @d=1> 

@lgierth lgierth referenced this issue in dkubb/axiom Nov 12, 2012

Closed

Breaks with activesupport 3.2.8 #15

Contributor

dkubb commented Nov 14, 2012

I think the reason for this is the DateTime#<=> method in lib/active_support/core_ext/date_time/calculations.rb. It should probably handle DateTime::Infinity instances differently, eg:

class DateTime
  # ...

  def <=>(other)
    return -1 if other.kind_of?(Infinity)
    super other.to_datetime
  end
end

Or it could delegate to super (Date#<=>) which handles DateTime::Infinity objects properly:

class DateTime
  # ...

  def <=>(other)                                                                 
    super other.kind_of?(Infinity) ? other : other.to_datetime                   
  end
end

A third option would be to define DateTime::Infinity#to_datetime and have it return self, which should allow the default comparison to return -1 as expected, eg:

class DateTime::Infinity
  # ...

  def to_datetime
    self
  end
end
Contributor

dkubb commented Dec 21, 2012

fwiw, I would happily submit a PR that fixes this bug if I could get some direction on which approach above I should take.

Owner

rafaelfranca commented Jan 14, 2013

@dkubb it is already fixed on master in a more generic way by #7350, but I prefer to not backport it.

I think we can go with your second solution on 3-2-stable.

Contributor

dkubb commented Jan 14, 2013

@rafaelfranca I opened a pull request that applies the fix you suggested. It also includes the tests from @gsphanikumar

Please ignore the previous PR, I mistakenly pointed it at rails/master.

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