Added ability to compare date/time with infinity #7350

Merged
merged 1 commit into from Jan 4, 2013

Projects

None yet

9 participants

Contributor
slbug commented Aug 14, 2012

Date, DateTime, Time and TimeWithZone can now be compared to infinity,
so it's now possible to create ranges with one infinite bound and
date/time object as another bound.

Ex.: @range = Range.new(Date.today, Float::INFINITY)

Also it's possible to check inclusion of date/time in range with
conversion.

Ex.: @range.include?(Time.now + 1.year) # => true
@range.include?(DateTime.now + 1.year) # => true

Ability to create date/time ranges with infinite bound is required
for handling postgresql range types.

Also it gives ability to create ranges from Time.now to Date.today + 10.years

Contributor
slbug commented Aug 14, 2012

Need feedback on this. May be rename some thing, or put in other places.Thanks.

/cc @rafaelfranca, @fxn, @tenderlove, @josevalim

@tenderlove tenderlove and 3 others commented on an outdated diff Aug 14, 2012
...rt/lib/active_support/core_ext/infinite_comparable.rb
@@ -0,0 +1,23 @@
+require 'active_support/concern'
+require 'active_support/core_ext/object/acts_like'
+
+module InfiniteComparable
+ extend ActiveSupport::Concern
+
+ included do
+ class_exec do
+ origin_compare = instance_method(:<=>)
+
+ define_method(:<=>) do |other|
tenderlove
tenderlove Aug 14, 2012 Owner

Why not just define the method and call super?

slbug
slbug Aug 14, 2012 Contributor

May be i did something wrong, but i tried it. And got infinite recursion by some reason (stack level too deep).

this way - https://gist.github.com/3352375
or this way - https://gist.github.com/3352429

just run those gists and you will see.

slbug
slbug Aug 14, 2012 Contributor

Yes, tried it too. No "stack level too deep", but it's not getting called at all. Just add some raise in method definition and you will see.

rafaelfranca
rafaelfranca Sep 20, 2012 Owner

I reviewed this one but don't figure it out why this is not being called. @jeremy @fxn any clues?

slbug
slbug Dec 18, 2012 Contributor

@tenderlove @rafaelfranca Looks like i understood why simple include does not work.

module A
  def foo
    "A"
  end
end

class B
  include A

  def foo
    "B"
  end
end

p B.new.foo   #=> "B"

And it won't work in this case too. And I think using included is only one option here until ruby 2.0 (Module#prepend)

slbug
slbug Dec 18, 2012 Contributor

https://gist.github.com/4326248 this one works too, but i don't this that this is really better than define_method

slbug
slbug Dec 22, 2012 Contributor

Okay, anyone can tell anything about this pull request? Tell me what I have to fix please.

/cc @dhh, @tenderlove, @steveklabnik, @sstephenson, @rafaelfranca, @jeremy, @fxn

rafaelfranca
rafaelfranca Dec 22, 2012 Owner

I would use the alias_method_chain technique for this case

rafaelfranca
rafaelfranca Dec 22, 2012 Owner

(sorry for the delay)

steveklabnik
steveklabnik Dec 22, 2012 Member

Yeah, it sucks to use it, but if super doesn't work...

slbug
slbug Dec 22, 2012 Contributor

Ok, I will try to use alias_method_chain, but I don't see any disadvantages in my code vs alias_method_chain.

tho, this is clear implementation. Documented behavior and works.

slbug
slbug Dec 22, 2012 Contributor

there is alias_method_chain imlementation https://gist.github.com/614ccbbe263cd1196203 looks almost same. will it be better if I commit this one?

/cc @dhh, @tenderlove, @steveklabnik, @sstephenson, @rafaelfranca, @jeremy, @fxn

rafaelfranca
rafaelfranca Dec 23, 2012 Owner
require 'active_support/concern'

module InfiniteComparable
  extend ActiveSupport::Concern

  included do
    alias_method_chain :<=>, :infinity
  end

  define_method '<=>_with_infinity' do |other|
    if other.class == self.class
      self.send(:'<=>_without_infinity', other)
    elsif other.respond_to?(:infinite?) && other.infinite?
      -other.infinite?
    else
      conversion = :"to_#{self.class.name.downcase}"

      other = other.send(conversion) if other.respond_to?(conversion)

      self.send(:'<=>_without_infinity', other)
    end
  end
end

WDYT?

@tenderlove tenderlove commented on an outdated diff Aug 14, 2012
...ctive_support/core_ext/numeric/infinite_comparable.rb
@@ -0,0 +1,17 @@
+require 'active_support/core_ext/big_decimal/conversions'
+require 'active_support/number_helper'
+
+class Numeric
+ [Float, BigDecimal].each do |klass|
+
+ origin_compare = klass.send(:instance_method, :<=>)
+
+ klass.send(:define_method , :<=>) do |other|
+ return origin_compare.bind(self).call(other) unless respond_to?(:infinite?)
+
+ return 0 if infinite? && other.respond_to?(:infinite?) && infinite? == other.infinite?
+
+ infinite? || origin_compare.bind(self).call(other)
tenderlove
tenderlove Aug 14, 2012 Owner

Same comment here. This is way too complex for what it needs to do.

Owner

Can you explain what this pull request is trying to accomplish? AFAIK, Date, DateTime, Float, etc can already be compared with Float::INFINITY. e.g:

irb(main):001:0> require 'date'
=> true
irb(main):002:0> Date.new <=> Float::INFINITY
=> -1
irb(main):003:0> DateTime.new <=> Float::INFINITY
=> -1
Contributor
slbug commented Aug 14, 2012
1.9.3p194 :001 > require 'date'
true
1.9.3p194 :002 > Date.new <=> Float::INFINITY
-1
1.9.3p194 :003 > DateTime.new <=> Float::INFINITY
-1
1.9.3p194 :004 > Time.new <=> Float::INFINITY
nil # fail
1.9.3p194 :005 > Float::INFINITY <=> Date.new
nil # fail
1.9.3p194 :006 > Float::INFINITY <=> DateTime.new
nil # fail
1.9.3p194 :007 > Float::INFINITY <=> Time.new
nil # fail
1.9.3p194 :008 >
@slbug slbug referenced this pull request Aug 15, 2012
Merged

Postgresql range support #7345

Contributor
slbug commented Aug 21, 2012

May be more reviews on it?

Contributor
slbug commented Sep 17, 2012

rebased this pull and #7345. can those pulls be merged or i still have to fix something?

/cc @rafaelfranca, @fxn, @tenderlove, @josevalim

Contributor
slbug commented Sep 18, 2012

added changelog entry

Member
seuros commented Oct 23, 2012

I'm using slbug's pullrequest for 3 weeks and everything is fine except that i cannot generate Date Range with -Infinity in the starting value.

@range = Range.new(-::Float::INFINITY,Date.today )
ArgumentError: bad value for range
Contributor
slbug commented Oct 23, 2012

@seuros that's strange because it works for me and tests pass for this one.

$ rails c
Loading development environment (Rails 4.0.0.beta)
[1] pry(main)> @range = Range.new(-::Float::INFINITY,Date.today )
=> -Infinity..Tue, 23 Oct 2012
[2] pry(main)> 
Member
seuros commented Oct 23, 2012

Sorry. I did the test in an unaltered Rails pull. Everything works perfectly. I hope this get merged soon.

Contributor
slbug commented Dec 27, 2012

@rafaelfranca, pull request updated.

P.S. There is one fail in activesupport tests which related to #8185

Contributor
slbug commented Dec 27, 2012

@rafaelfranca updated again. Removed some stuff which not required. Rebased against master

Owner

I'll take a look later

joallard commented Jan 3, 2013

@rafaelfranca Any news?

@rafaelfranca rafaelfranca commented on the diff Jan 4, 2013
activesupport/test/core_ext/numeric_ext_test.rb
+ assert_equal(1, @inf <=> DateTime.now)
+ end
+ end
+
+ def test_compare_infinity_with_twz
+ time_zone = ActiveSupport::TimeZone['Eastern Time (US & Canada)']
+ twz = ActiveSupport::TimeWithZone.new(Time.now, time_zone)
+
+ assert_nothing_raised do
+ assert_equal(-1, -Float::INFINITY <=> twz)
+ assert_equal(1, Float::INFINITY <=> twz)
+ assert_equal(-1, -@inf <=> twz)
+ assert_equal(1, @inf <=> twz)
+ end
+ end
+
@rafaelfranca rafaelfranca commented on an outdated diff Jan 4, 2013
activesupport/lib/active_support/core_ext/time.rb
@@ -3,3 +3,5 @@
require 'active_support/core_ext/time/conversions'
require 'active_support/core_ext/time/marshal'
require 'active_support/core_ext/time/zones'
+require 'active_support/core_ext/time/infinite_comparable'
+
@rafaelfranca rafaelfranca commented on an outdated diff Jan 4, 2013
...ctive_support/core_ext/numeric/infinite_comparable.rb
@@ -0,0 +1,12 @@
+require 'active_support/core_ext/big_decimal/conversions'
+require 'active_support/number_helper'
+require 'active_support/core_ext/infinite_comparable'
+
+class Float
+ include InfiniteComparable
+end
+
+class BigDecimal
+ include InfiniteComparable
+end
+
@rafaelfranca rafaelfranca commented on the diff Jan 4, 2013
...rt/lib/active_support/core_ext/infinite_comparable.rb
+ infinite? <=> other.infinite?
+ # not_inf <=> inf
+ elsif other.respond_to?(:infinite?) && other.infinite?
+ -other.infinite?
+ # inf <=> not_inf
+ elsif respond_to?(:infinite?) && infinite?
+ infinite?
+ else
+ conversion = :"to_#{self.class.name.downcase}"
+
+ other = other.send(conversion) if other.respond_to?(conversion)
+
+ self.send(:'<=>_without_infinity', other)
+ end
+ end
+
@rafaelfranca rafaelfranca commented on the diff Jan 4, 2013
...rt/lib/active_support/core_ext/infinite_comparable.rb
@@ -0,0 +1,32 @@
+require 'active_support/concern'
+
+module InfiniteComparable
+ extend ActiveSupport::Concern
+
+ included do
+ alias_method_chain :<=>, :infinity
+ end
+
+ define_method '<=>_with_infinity' do |other|
+
@rafaelfranca rafaelfranca commented on an outdated diff Jan 4, 2013
activesupport/lib/active_support/core_ext/date_time.rb
@@ -2,3 +2,5 @@
require 'active_support/core_ext/date_time/calculations'
require 'active_support/core_ext/date_time/conversions'
require 'active_support/core_ext/date_time/zones'
+require 'active_support/core_ext/date_time/infinite_comparable'
+
Owner

It needs a rebase and remove all the lines that I added a ✂️

Other than that it is good to merge

@slbug slbug Added ability to compare date/time with infinity
Date, DateTime, Time and TimeWithZone can now be compared to infinity,
  so it's now possible to create ranges with one infinite bound and
  date/time object as another bound.

  Ex.: @range = Range.new(Date.today, Float::INFINITY)

Also it's possible to check inclusion of date/time in range with
  conversion.

  Ex.: @range.include?(Time.now + 1.year) # => true
       @range.include?(DateTime.now + 1.year) # => true

Ability to create date/time ranges with infinite bound is required
  for handling postgresql range types.
38f28dc
Contributor
slbug commented Jan 4, 2013

@rafaelfranca fixed & rebased

@rafaelfranca rafaelfranca merged commit e3ce5ea into rails:master Jan 4, 2013
Owner

Thank you and sorry for the delay

joallard commented Jan 4, 2013

Awesome!

Contributor
slbug commented May 3, 2013

@tenderlove, why it was reverted?

Owner
jeremy commented May 3, 2013
  1. Float::INFINITY is a float, not a date or time.
  2. Infinity != Infinity. Infinities are not comparable.
  3. This implementation caused everything to be comparable with dates/times, incurring a costly to_datetime coercion attempt. With String, for example:
2.1.0-dev ~ ruby -Ilib -ractive_support/all -e'$DEBUG=true; puts DateTime.now == "hello"'
Exception `ArgumentError' at /Users/jeremy/.rbenv/versions/2.1.0-dev/lib/ruby/gems/2.1.0/gems/activesupport-4.0.0.beta1/lib/active_support/core_ext/string/conversions.rb:55 - invalid date
false

Flip the comparison, no coercion attempt:

2.1.0-dev ~ ruby -Ilib -ractive_support/all -e'$DEBUG=true; puts "hello" == DateTime.now'
false
Contributor
slbug commented May 3, 2013

@jeremy

  1. yes. we can just add Date::INFINITY, etc
  2. yes in math, but not in ruby.
    ruby -e 'p Float::INFINITY == Float::INFINITY'
    true
    
  3. agree, this is a problem. but can be fixed. should i give it another try?

@jeremy @slbug Removing this patch now causes PostgreSQL tstzrange columns with infinite from (or to) to fail with a 'bad value for range' argument error. The failure came between rc1 and rc2 (I assume by removing activesupport/lib/active_support/core_ext/infinite_comparable.rb). Any fixes for this problem would be greatly appreciated.

Contributor

Hm, seeing:

irb(main):012:0> Time.zone.now
=> Wed, 02 Apr 2014 10:17:10 PDT -07:00

irb(main):013:0> Time.zone.now > Float::INFINITY
ArgumentError: comparison of ActiveSupport::TimeWithZone with Float failed
    from (irb):13:in `>'
    from (irb):13
    from /usr/local/stow/ruby-2.1.1-gh-2014.3.18/lib/ruby/gems/2.1.0/gems/railties-4.0.4/lib/rails/commands/console.rb:90:in `start'
    from /usr/local/stow/ruby-2.1.1-gh-2014.3.18/lib/ruby/gems/2.1.0/gems/railties-4.0.4/lib/rails/commands/console.rb:9:in `start'
    from /usr/local/stow/ruby-2.1.1-gh-2014.3.18/lib/ruby/gems/2.1.0/gems/railties-4.0.4/lib/rails/commands.rb:62:in `<top (required)>'
    from ./bin/rails:4:in `require'
    from ./bin/rails:4:in `<main>'
Contributor

For people who are trying to compare to infinity, this pull request was reverted by 6c61dbf

@robin850 robin850 referenced this pull request Apr 6, 2014
@tenderlove tenderlove Squashed commit of the following:
commit 2683de5da85135e8d9fe48593ff6167db9d64b18
Author: Aaron Patterson <aaron.patterson@gmail.com>
Date:   Fri May 3 11:29:20 2013 -0700

    cannot support infinite ranges right now

commit cebb6acef2c3957f975f6db4afd849e535126253
Author: Aaron Patterson <aaron.patterson@gmail.com>
Date:   Fri May 3 11:26:12 2013 -0700

    reverting infinity comparison

commit 385f7e6b4efd1bf9b89e8d607fcb13e5b03737ea
Author: Aaron Patterson <aaron.patterson@gmail.com>
Date:   Fri May 3 11:23:28 2013 -0700

    Revert "Added ability to compare date/time with infinity"

    This reverts commit 38f28dc.

    Conflicts:
    	activesupport/CHANGELOG.md
    	activesupport/lib/active_support/core_ext/numeric/infinite_comparable.rb
    	activesupport/test/core_ext/date_ext_test.rb
    	activesupport/test/core_ext/date_time_ext_test.rb
    	activesupport/test/core_ext/numeric_ext_test.rb
    	activesupport/test/core_ext/time_ext_test.rb
    	activesupport/test/core_ext/time_with_zone_test.rb

commit 0d799a188dc12b18267fc8421675729917610047
Author: Aaron Patterson <aaron.patterson@gmail.com>
Date:   Fri May 3 11:18:53 2013 -0700

    Revert "Refactor infinite comparable definition a bit"

    This reverts commit dd3360e.

commit 42dec90e49745bbfae546f0560b8783f6b48b074
Author: Aaron Patterson <aaron.patterson@gmail.com>
Date:   Fri May 3 11:18:47 2013 -0700

    Revert "Require 'active_support/core_ext/module/aliasing' in the infinite_comparable module"

    This reverts commit 7003e71.
6c61dbf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment