range of Time has trouble seeing if Date object is included #2588

Closed
pivotalcommon opened this Issue Aug 19, 2011 · 24 comments

Comments

Projects
None yet
@pivotalcommon

rails 3.0.9

Example:

range = 10.day.ago..2.days.ago
range.include?(Date.today) #should be false

We get an infinite loop of the following stacktrace:

./gems/activesupport-3.0.9/lib/active_support/time_with_zone.rb:322: warning: Time#succ is obsolete; use time + 1
./gems/activesupport-3.0.9/lib/active_support/core_ext/object/acts_like.rb:8:in `call'
./gems/activesupport-3.0.9/lib/active_support/core_ext/object/acts_like.rb:8:in `acts_like?'
./gems/activesupport-3.0.9/lib/active_support/core_ext/time/calculations.rb:278:in `compare_with_coercion'
./gems/activesupport-3.0.9/lib/active_support/time_with_zone.rb:183:in `<=>'
./gems/activesupport-3.0.9/lib/active_support/core_ext/range/include_range.rb:16:in `each'
./gems/activesupport-3.0.9/lib/active_support/core_ext/range/include_range.rb:16:in `include?'
./gems/activesupport-3.0.9/lib/active_support/core_ext/range/include_range.rb:16:in `include?'
./gems/activesupport-3.0.9/lib/active_support/core_ext/range/include_range.rb:16:in `include_with_range?'

Are we to expect the a Time range includes a Date?

NOTE: We are using home_run, but we were able to verify that the same error occurs with and without home_run.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Nov 14, 2011

Contributor

This also bit me today. What's the best way to write a test that catches an infinite loop?

Contributor

tekin commented Nov 14, 2011

This also bit me today. What's the best way to write a test that catches an infinite loop?

@alindeman

This comment has been minimized.

Show comment
Hide comment
@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Nov 14, 2011

Contributor

LOL.

Having looked into this, I'm not sure it is actually infinite looping - it's just checking every second between 10 days and 2 days ago incrementally, which isn't a particularly efficient manner in which to test if a date is in a range. The obvious solution is to build your range with Dates rather than TimeWithZones, that'll give you what you want and not take forever.

Contributor

tekin commented Nov 14, 2011

LOL.

Having looked into this, I'm not sure it is actually infinite looping - it's just checking every second between 10 days and 2 days ago incrementally, which isn't a particularly efficient manner in which to test if a date is in a range. The obvious solution is to build your range with Dates rather than TimeWithZones, that'll give you what you want and not take forever.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Nov 15, 2011

Contributor

This does bring up the issue of what to do about Time ranges though - Time#succ is now obsolete in Rails 1.9.2+ and Range relies on the #succ to step through a range.

Contributor

tekin commented Nov 15, 2011

This does bring up the issue of what to do about Time ranges though - Time#succ is now obsolete in Rails 1.9.2+ and Range relies on the #succ to step through a range.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins Apr 29, 2012

range = 10.day.ago.to_i..2.days.ago.to_i
range.include?(Date.today.to) #will be false

I think this can be closed now.

range = 10.day.ago.to_i..2.days.ago.to_i
range.include?(Date.today.to) #will be false

I think this can be closed now.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2012

Member

@twinturbo without the .to_i, it does still blow up.

Member

steveklabnik commented Apr 30, 2012

@twinturbo without the .to_i, it does still blow up.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins Apr 30, 2012

@steveklabnik it should work using to_i. Are you saying it doesn't?

@steveklabnik it should work using to_i. Are you saying it doesn't?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 30, 2012

Member

With the to_i: no crash. Without it: crash.

Member

steveklabnik commented Apr 30, 2012

With the to_i: no crash. Without it: crash.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins Apr 30, 2012

So the question is: what should we do? Should we patch Date/Time/DateTime ranges to use .to_i for include? or what?

cc @rafaelfranca

So the question is: what should we do? Should we patch Date/Time/DateTime ranges to use .to_i for include? or what?

cc @rafaelfranca

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 30, 2012

Member

I'll leave this decision to the Rails core cc/ @fxn @jeremy

Member

rafaelfranca commented Apr 30, 2012

I'll leave this decision to the Rails core cc/ @fxn @jeremy

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 30, 2012

Member

Think this should "just work" without converting to_i or hitting an infinite loops, for sure!

Member

jeremy commented Apr 30, 2012

Think this should "just work" without converting to_i or hitting an infinite loops, for sure!

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Apr 30, 2012

Contributor

I haven't checked recently, but if I recall correctly, the issue was that it took an age to return (giving the appearance of having crashed) if you passed in a Time Range because the underlying code was iterating through every second between the two limits of the range, creating a new Time object each time.

Contributor

tekin commented Apr 30, 2012

I haven't checked recently, but if I recall correctly, the issue was that it took an age to return (giving the appearance of having crashed) if you passed in a Time Range because the underlying code was iterating through every second between the two limits of the range, creating a new Time object each time.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 30, 2012

Member

Yeah - that's insane ;)

Member

jeremy commented Apr 30, 2012

Yeah - that's insane ;)

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins Apr 30, 2012

I don't think there is anyway to fix without changing the way the objects work with ranges. Using to_i seems easy enough IMO. I'd like to see it "just work", but I have no clue how that can happen.

I don't think there is anyway to fix without changing the way the objects work with ranges. Using to_i seems easy enough IMO. I'd like to see it "just work", but I have no clue how that can happen.

@mathieuseguin

This comment has been minimized.

Show comment
Hide comment
@mathieuseguin

mathieuseguin May 7, 2012

using to_i is not ideal. It stops you from using chained method such as 2.days.ago.beginning_of_day..2.days.ago.end_of_day

using to_i is not ideal. It stops you from using chained method such as 2.days.ago.beginning_of_day..2.days.ago.end_of_day

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins May 7, 2012

@ozgor no it doesn't You chose to when to use #to_i. 2.days.ago.beginning_of_day.to_i...2.days.ago.end_of_day.to_i

ahawkins commented May 7, 2012

@ozgor no it doesn't You chose to when to use #to_i. 2.days.ago.beginning_of_day.to_i...2.days.ago.end_of_day.to_i

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin May 7, 2012

Contributor

@twinturbo all the same, calling #to_i is not really an optimal solution - it relies on user knowledge of this bug rather than it simply "just working".

The question I'm wondering: Is it even valid to call #include? on a Time range? Time is not a discrete unit, but somewhere in the belly of either ActiveSupport, tzinfo or the Standard Lib itself, a time range gets enumerated through if you call #include?, which raises lots of warnings (because Time#succ is deprecated) and ends up taking forever.

I've got around this issue by calling #cover? instead (http://www.ruby-doc.org/core-1.9.3/Range.html#method-i-cover-3F) which seems a more sensible method, but all the same, maybe it should be expected that #include? should just work, even for a TimeWithZone range..

Contributor

tekin commented May 7, 2012

@twinturbo all the same, calling #to_i is not really an optimal solution - it relies on user knowledge of this bug rather than it simply "just working".

The question I'm wondering: Is it even valid to call #include? on a Time range? Time is not a discrete unit, but somewhere in the belly of either ActiveSupport, tzinfo or the Standard Lib itself, a time range gets enumerated through if you call #include?, which raises lots of warnings (because Time#succ is deprecated) and ends up taking forever.

I've got around this issue by calling #cover? instead (http://www.ruby-doc.org/core-1.9.3/Range.html#method-i-cover-3F) which seems a more sensible method, but all the same, maybe it should be expected that #include? should just work, even for a TimeWithZone range..

@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Sep 12, 2012

Contributor

@jeremy can we close this?

Contributor

frodsan commented Sep 12, 2012

@jeremy can we close this?

@tomrossi7

This comment has been minimized.

Show comment
Hide comment
@tomrossi7

tomrossi7 Sep 20, 2012

I just ran into this today as well with tests that compare date/times to see if something is included. Example:

assert (1.minutes.ago..Time.now).include?(object.created_at)

I just ran into this today as well with tests that compare date/times to see if something is included. Example:

assert (1.minutes.ago..Time.now).include?(object.created_at)
@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Mar 25, 2013

Contributor

This is a really long running issue with many workarounds and no accepted solution.

Should it remain open as an issue with Rails? If so, what sort of patch are we looking for?

Contributor

parndt commented Mar 25, 2013

This is a really long running issue with many workarounds and no accepted solution.

Should it remain open as an issue with Rails? If so, what sort of patch are we looking for?

@adamthedeveloper

This comment has been minimized.

Show comment
Hide comment
@adamthedeveloper

adamthedeveloper Apr 29, 2013

I've had this come up recently on this line:

gems/activesupport-3.2.13.rc1/lib/active_support/time_with_zone.rb:332: warning: Time#succ is obsolete; use time + 1

Same issue?

I've had this come up recently on this line:

gems/activesupport-3.2.13.rc1/lib/active_support/time_with_zone.rb:332: warning: Time#succ is obsolete; use time + 1

Same issue?

@wyaeld

This comment has been minimized.

Show comment
Hide comment
@wyaeld

wyaeld Jun 12, 2013

Contributor

yes, its the same issue, not a loop, but its checking for every second in the range. main workarounds seem to be to call to_i or to_date beforehand depending on your comparison.

Contributor

wyaeld commented Jun 12, 2013

yes, its the same issue, not a loop, but its checking for every second in the range. main workarounds seem to be to call to_i or to_date beforehand depending on your comparison.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca commented Jul 16, 2013

See #11463

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jul 17, 2013

Member

Note if you want to use Range#cover? to check if a Date is inside a ActiveSupport::TimeWithZone you'll need to convert it to a ActiveSupport::TimeWithZone first, e.g:

# Rails 3.2
>> range = 10.days.ago..10.days.from_now
=> Sun, 07 Jul 2013 04:42:42 BST +01:00..Sat, 27 Jul 2013 04:42:42 BST +01:00
>> range.cover?(Date.current.to_time_in_current_zone)
=> true

# Rails 4.0
>> range = 10.days.ago..10.days.from_now
=> Sun, 07 Jul 2013 04:42:42 BST +01:00..Sat, 27 Jul 2013 04:42:42 BST +01:00
>> range.cover?(Date.current.in_time_zone)
=> true
Member

pixeltrix commented Jul 17, 2013

Note if you want to use Range#cover? to check if a Date is inside a ActiveSupport::TimeWithZone you'll need to convert it to a ActiveSupport::TimeWithZone first, e.g:

# Rails 3.2
>> range = 10.days.ago..10.days.from_now
=> Sun, 07 Jul 2013 04:42:42 BST +01:00..Sat, 27 Jul 2013 04:42:42 BST +01:00
>> range.cover?(Date.current.to_time_in_current_zone)
=> true

# Rails 4.0
>> range = 10.days.ago..10.days.from_now
=> Sun, 07 Jul 2013 04:42:42 BST +01:00..Sat, 27 Jul 2013 04:42:42 BST +01:00
>> range.cover?(Date.current.in_time_zone)
=> true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment