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

Deprecate using `Range#include?` to check the inclusion of a value in a date time range #38186

Conversation

@vishaltelangre
Copy link
Contributor

vishaltelangre commented Jan 8, 2020

Summary

The usage of the Range#include? method to check the inclusion of an argument in date-time with zone range is deprecated. This method is extended in active_support/core_ext/range/include_time_with_zone extension as of now.

As a replacement, it is recommended to use Range#cover? instead of Range#include? to check the inclusion of a value in a date-time range.

Reference: #36194 (comment) cc: @rafaelfranca

@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch from d5b921d to 402bdc2 Jan 8, 2020
@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch 3 times, most recently from d300870 to d1a16d4 Jan 8, 2020
@@ -1,6 +1,16 @@
# frozen_string_literal: true

require "active_support/time_with_zone"
require "active_support/deprecation"

ActiveSupport::Deprecation.warn(<<-MSG.squish)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jan 8, 2020

Member

Before deprecating the file we need to deprecate the method itself. So instead of this patch we should should deprecations on line 23 and line 25 when the conditionals for self.begin.is_a?(TimeWithZone) and self.end.is_a?(TimeWithZone) are executed telling people to use cover? instead of include?

This comment has been minimized.

Copy link
@vishaltelangre

vishaltelangre Jan 8, 2020

Author Contributor

@rafaelfranca I have updated the PR accordingly.

@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch from d1a16d4 to 2cc6777 Jan 8, 2020
@vishaltelangre vishaltelangre changed the title Deprecate requiring `active_support/core_ext/range/include_time_with_zone` Deprecate using `Range#include?` to check the inclusion of a value in a date time range Jan 8, 2020
@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch from 2cc6777 to a914f40 Jan 8, 2020
@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch from a914f40 to b30575e Jan 8, 2020
activesupport/CHANGELOG.md Outdated Show resolved Hide resolved
… a date time range

Use `Range#cover?` instead of `Range#include?` to check the inclusion of a value in a date time range.
@vishaltelangre vishaltelangre force-pushed the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch from b30575e to 740bb17 Jan 8, 2020
@rafaelfranca rafaelfranca merged commit ff341ae into rails:master Jan 8, 2020
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #66281 passed (13 minutes, 13 seconds)
Details
@vishaltelangre vishaltelangre deleted the vishaltelangre:deprecate-AS-range-include_time_with_zone-ext branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.