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

Fix: Inclusion of DateTime in a Date range and vice versa #36194

Conversation

@vishaltelangre
Copy link
Contributor

vishaltelangre commented May 7, 2019

Fixes #36175 which exists for Rails 5.2.3 and Rails 6, too.

With this fix, the range inclusion predicate method works as intended.

(Date.today.beginning_of_month..Date.today.end_of_month).include?(Date.today) # => true
(Date.today.beginning_of_month..Date.today.end_of_month).include?(DateTime.now) # => true
(DateTime.now.beginning_of_month..DateTime.now.end_of_month).include?(Date.today) # => true
(DateTime.now.beginning_of_month..DateTime.now.end_of_month).include?(DateTime.now) # => true
@rails-bot rails-bot bot added the activesupport label May 7, 2019
range = DateTime.now.beginning_of_month..DateTime.now.end_of_month
assert(range.include?(DateTime.now))
end

This comment has been minimized.

Copy link
@st0012

st0012 May 7, 2019

Contributor

Should also add test cases for Time inclusion. Like:

(Date.yesterday..Date.tomorrow).include?(Time.now)
(Date.yesterday..Date.tomorrow).include?(Time.zone.now)

This comment has been minimized.

Copy link
@vishaltelangre

vishaltelangre May 7, 2019

Author Contributor

Added tests for these scenarios, too.

… versa
@rails-bot

This comment has been minimized.

Copy link

rails-bot bot commented Dec 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 17, 2019
@st0012

This comment has been minimized.

Copy link
Contributor

st0012 commented Dec 20, 2019

@vishaltelangre I've opened an issue report on Ruby to check if the change should be done at Ruby level instead: https://bugs.ruby-lang.org/issues/16440

In the meantime, I think the PR looks good. Can you squash your commits so once they make the call it can easily be merged?

@rails-bot rails-bot bot removed the stale label Dec 20, 2019
@zverok

This comment has been minimized.

Copy link
Contributor

zverok commented Dec 21, 2019

Range#include? in Ruby checks if the argument is one of the members of the range-as-a-sequence, e.g. if Range#to_a will include it.
To check whether the argument is in the space between begin and end, Range#cover? should be used.

I believe this kind of "edge case hacks", even if they look good on one particular example, are in fact harmful (for AS users) because of muddying the underlying Ruby semantics.

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

vishaltelangre commented Dec 29, 2019

@st0012 Is this PR still a candidate to be merged?

@st0012

This comment has been minimized.

Copy link
Contributor

st0012 commented Dec 29, 2019

@vishaltelangre unfortunately, it's not my call 😬.
Based on what I see now, it probably won't be merged, unfortunately. If they decide the behavior is valid, they will implement it at the Ruby level instead of Rails. And if they don't think this is expected behavior, Rails maintainers probably won't want to support it in Rails either, unless there's a strong argument for having this feature at the Rails level.

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

vishaltelangre commented Dec 29, 2019

@st0012 Sounds good to me. I will leave this PR as is. It will be automatically closed by @rails-bot once it becomes stale.

@st0012

This comment has been minimized.

Copy link
Contributor

st0012 commented Jan 7, 2020

@vishaltelangre my bug report in Ruby has been rejected

include? should only be used if you want to check the argument is one of the members of the range (i.e. included in the array returned by to_a)

So I guess we should follow the same standard in Rails.

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

vishaltelangre commented Jan 7, 2020

@st0012 Thanks for the update. In that case, should we close this fix as well?

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jan 7, 2020

I agree with @zverok here. Does cover? work for the examples you gave in the description?

if first.is_a?(Date) || first.is_a?(DateTime)
cover?(value)
elsif last.is_a?(Date) || last.is_a?(DateTime)
cover?(value)

This comment has been minimized.

Copy link
@vishaltelangre

vishaltelangre Jan 7, 2020

Author Contributor

@rafaelfranca That's what this PR does. We rely on #cover? here. This fix is inspired by this extension https://github.com/rails/rails/blob/870f087825/activesupport/lib/active_support/core_ext/range/include_time_with_zone.rb which is already present in the upstream.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 7, 2020

Member

I'm more inclined to remove that extension. include? should not work for values that Range.to_a would not return.

Can we deprecate that extension and tell people to use cover? instead?

This comment has been minimized.

Copy link
@vishaltelangre

vishaltelangre Jan 7, 2020

Author Contributor

@rafaelfranca Sure. I will make another PR to deprecate the activesupport/lib/active_support/core_ext/range/include_time_with_zone.rb extension.

@vishaltelangre

This comment has been minimized.

Copy link
Contributor Author

vishaltelangre commented Jan 7, 2020

Closing this PR since it is encouraged to use the #cover? method to check the inclusion of DateTime in a Date range and vice versa instead of using the #include? method to do the same.

@vishaltelangre vishaltelangre deleted the vishaltelangre:date-and-date-time-range-include-fix branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.