Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime range include? returns wrong output (or so I think) #30799

Closed
apneadiving opened this issue Oct 4, 2017 · 8 comments
Closed

DateTime range include? returns wrong output (or so I think) #30799

apneadiving opened this issue Oct 4, 2017 · 8 comments

Comments

@apneadiving
Copy link

apneadiving commented Oct 4, 2017

Steps to reproduce

ref = DateTime.now
ts1 = (ref - 1.hour)..(ref + 1.hour)
ts2 = (ref)..(ref + 1.hour)
ts1.include? ts2.first
> false

Expected behavior

ts1.include? ts2.first should return true

Actual behavior

ts1.include? ts2.first returns false

Side notes

Interestingly enough, if you use Time instead of DateTime, it works as expected.

With Ruby only, it does not work...

require 'date'
ref = DateTime.now
ts1 = (ref - Rational(3600, 86400))..(ref + Rational(3600, 86400))
ts2 = (ref)..(ref + Rational(3600, 86400))
ts1.include? ts2.first
> false

System configuration

Rails version: 5.1.3
Ruby version: 2.4.2

@apneadiving apneadiving changed the title DateTime range include? returns wrong output (or so I think) DateTime range include? returns wrong output (or so I think) Oct 4, 2017
@nicolasblanco
Copy link

On your use case Range#include? does not return what you expect simply because a range of two DateTimes is not converted to an array containing all the dates with a granularity of a second.

You need to use Range#cover? to check if a value is between your range of two DateTimes.

@apneadiving
Copy link
Author

apneadiving commented Oct 4, 2017

cover? works but... the whole point is to do something like: ts1.include?(ts2).

ActiveSupport extends include? properly to check range to range. Internally it checks: ts1.include?(ts2.first) && ts1.last >= ts2.last.

So it fails because of the ts1.include?(ts2.first)...

What is also confusing is that it works with Time but not DateTime.

@nicolasblanco
Copy link

@apneadiving : have a look at:
https://github.com/rails/rails/blob/v5.1.4/activesupport/lib/active_support/core_ext/range/include_range.rb

Range#include? is only extended in ActiveSupport to support a Range as a parameter. If something different than a Range is passed, the original behaviour is applied.

@apneadiving
Copy link
Author

@nicolasblanco indeed this is what I describe :) it works fine with Time but not with DateTime.

@nicolasblanco
Copy link

@apneadiving

About your question on why you have a different behaviour between a range of DateTimes and a range of Times:

Ruby has a customized way of dealing with a range of Times. For this reason, you cannot convert a range of Times to an array:

(Time.now..3.hours.from_now).to_a
#=> TypeError: can't iterate from Time

But you can convert a range of DateTimes to an array:

(DateTime.now..(DateTime.now + 3.days)).to_a
#=> [Wed, 04 Oct 2017 16:49:43 +0200, Thu, 05 Oct 2017 16:49:43 +0200, Fri, 06 Oct 2017 
16:49:43 +0200, Sat, 07 Oct 2017 16:49:43 +0200]

As you see, when converting a range of DateTimes to an array, Ruby has a granularity of 1 day between each DateTime.

That's why you don't have the expected behaviour when using Range#include? with DateTime values...

@apneadiving
Copy link
Author

The whole point of my issue is questionning the inconsistency of what include? returns between Time and DateTime.
Hopefuly it would be considered as a legitimate request.

@al2o3cr
Copy link
Contributor

al2o3cr commented Oct 4, 2017

Looks like DateTime should be considered a "linear object" like Time and Numeric are in this patch:

https://bugs.ruby-lang.org/issues/11113

Since it isn't, the default include? implementation from Enumerable gets used. The default behavior of Range performs each using succ, which increments a DateTime by a full day. :(

@pixeltrix
Copy link
Contributor

The issue with DateTime ranges should be fixed in Ruby itself by removing the succ method and making include? use cover? like core Ruby does for floats. However ActiveSupport::TimeWithZone does not have a succ method but include? doesn't work because the linear object check is coded to a fixed set of core types - this we can fix in Active Support.

pixeltrix added a commit that referenced this issue Nov 7, 2017
In #11474 we prevented TWZ ranges being iterated over which matched
Ruby's handling of Time ranges and as a consequence `include?` stopped
working with both Time ranges and TWZ ranges. However in
ruby/ruby@b061634 support was added for `include?` to use `cover?` for
'linear' objects. Since we have no way of making Ruby consider TWZ
instances as 'linear' we have to override `Range#include?`.

Fixes #30799.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants