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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions activesupport/lib/active_support/core_ext/range.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
require "active_support/core_ext/range/conversions"
require "active_support/core_ext/range/compare_range"
require "active_support/core_ext/range/include_time_with_zone"
require "active_support/core_ext/range/include_date_time"
require "active_support/core_ext/range/overlaps"
require "active_support/core_ext/range/each"
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module ActiveSupport
module IncludeDateTime #:nodoc:
# Extends the default Range#include? to support
# Date and DateTime range comparisons.
#
# (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
#
def include?(value)
if first.is_a?(Date) || first.is_a?(DateTime)
cover?(value)
elsif last.is_a?(Date) || last.is_a?(DateTime)
cover?(value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
super
end
end
end
end

Range.prepend(ActiveSupport::IncludeDateTime)
53 changes: 53 additions & 0 deletions activesupport/test/core_ext/range_ext_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,59 @@ def test_date_range
assert_instance_of Range, DateTime.new..DateTime::Infinity.new
end

def test_date_range_includes_date
range = Date.today.beginning_of_month..Date.today.end_of_month
assert(range.include?(Date.today))
end

def test_date_range_includes_date_time
range = Date.today.beginning_of_month..Date.today.end_of_month
assert(range.include?(DateTime.now))
end

def test_date_time_range_includes_date
range = DateTime.now.beginning_of_month..DateTime.now.end_of_month
assert(range.include?(Date.today))
end

def test_date_time_range_includes_date_time
range = DateTime.now.beginning_of_month..DateTime.now.end_of_month
assert(range.include?(DateTime.now))
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also add test cases for Time inclusion. Like:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for these scenarios, too.

def test_date_range_includes_time
range = Date.yesterday..Date.tomorrow

assert(range.include?(Time.now))
end

def test_date_range_includes_time_with_zone
time_with_zone = ActiveSupport::TimeWithZone.new(nil, ActiveSupport::TimeZone["UTC"], Time.now.utc)
range = Date.yesterday..Date.tomorrow

assert(range.include?(time_with_zone))
end

def test_date_time_range_includes_time
range = DateTime.yesterday..DateTime.tomorrow

assert(range.include?(Time.now))
end

def test_time_with_zone_range_includes_time_with_zone
yesterday = ActiveSupport::TimeWithZone.new(nil, ActiveSupport::TimeZone["UTC"], 1.day.ago)
time_with_zone = ActiveSupport::TimeWithZone.new(nil, ActiveSupport::TimeZone["UTC"], Time.now.utc)
range = (yesterday..1.minute.from_now)

assert(range.include?(time_with_zone))
end

def test_time_with_zone_range_includes_date
range = (1.week.ago..2.days.from_now)

assert(range.include?(Date.today))
end

def test_overlaps_last_inclusive
assert((1..5).overlaps?(5..10))
end
Expand Down