Skip to content

Loading…

Extended method DateTime#<=> can accept nil #10595

Closed
wants to merge 2 commits into from

5 participants

@soylent

Bug: extended method DateTime#<=> does not accept nil

This causes the following problem:

% rails console
irb(main):001:0> (Time.now..Time.now).cover? nil
NoMethodError: undefined method `to_datetime' for nil:NilClass
    from /Users/Soilent/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-b44f086cf105/activesupport/lib/active_support/core_ext/date_time/calculations.rb:162:in `<=>'
    from /Users/Soilent/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-b44f086cf105/activesupport/lib/active_support/core_ext/time/calculations.rb:269:in `compare_with_coercion'
    from (irb):1:in `cover?'
    from (irb):1
    from /Users/Soilent/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-b44f086cf105/railties/lib/rails/commands/console.rb:90:in `start'
    from /Users/Soilent/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-b44f086cf105/railties/lib/rails/commands/console.rb:9:in `start'
    from /Users/Soilent/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/bundler/gems/rails-b44f086cf105/railties/lib/rails/commands.rb:66:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

Original method DateTime#<=> accepts nil, and this problem does not happen:

% irb
irb(main):001:0> require 'date'
=> true
irb(main):002:0> (Time.now..Time.now).cover? nil
=> false
irb(main):003:0> DateTime.new <=> nil
=> nil
@dmathieu

This makes the usage of try potentially necessary every time we call to_datetime.
I think it would be more stable to have the method to_datetime defined in NilClass, to return nil.

@pixeltrix
Ruby on Rails member

Actually, I think a proper refactoring is in order. The to_datetime coercion was because Time in earlier versions of Ruby was restricted to a limited range. Now that has gone away in 1.9.3 we can coerce to a Time instance instead of DateTime. All of the _with_coercion methods in the date and time core extensions should be revisited.

@tamird

Looks like this is fixed. Good to close.

reuqire '2.1.2 :001 > require 'active_support/all'
 => true
2.1.2 :002 > (Time.now..Time.now).cover? nil
 => false

Looks like it was coincidentally fixed in b0acc77 cc @tenderlove

@lukaszx0
Ruby on Rails member

Closing. @soylent let me know if this is still an issue for you.

@lukaszx0 lukaszx0 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 13, 2013
  1. @soylent

    DateTime#<=> can accept nil

    soylent committed
Commits on Jul 31, 2013
  1. @soylent
View
2 activesupport/lib/active_support/core_ext/date_time/calculations.rb
@@ -161,7 +161,7 @@ def utc_offset
# Layers additional behavior on DateTime#<=> so that Time and
# ActiveSupport::TimeWithZone instances can be compared with a DateTime.
def <=>(other)
- super other.to_datetime
+ super other.try(:to_datetime)
end
end
View
4 activesupport/test/core_ext/date_time_ext_test.rb
@@ -327,6 +327,10 @@ def test_compare_with_time_with_zone
assert_equal(-1, DateTime.civil(2000) <=> ActiveSupport::TimeWithZone.new( Time.utc(2000, 1, 1, 0, 0, 1), ActiveSupport::TimeZone['UTC'] ))
end
+ def test_compare_with_nil
+ assert_nil DateTime.new <=> nil
+ end
+
def test_to_f
assert_equal 946684800.0, DateTime.civil(2000).to_f
assert_equal 946684800.0, DateTime.civil(1999,12,31,19,0,0,Rational(-5,24)).to_f
Something went wrong with that request. Please try again.