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

Allow Range#=== and Range#cover? on Range #32938

Merged
merged 2 commits into from May 22, 2018

Conversation

Projects
None yet
6 participants
@utilum
Contributor

utilum commented May 20, 2018

ruby/ruby@989e07c features switching Range#=== to use internal r_cover_p
instead of rubyland include?. This breaks expected behavior of
ActiveSupport::CoreExt::Range documented since at least 8b67a02.

This patch adds overrides on Range#cover? and Range#=== and places all
three in a single module, CompareWithRange.
See failures at:
https://travis-ci.org/rails/rails/jobs/380939901#L1224-L1247

@rails-bot

This comment has been minimized.

rails-bot commented May 20, 2018

r? @kamipo

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd

This comment has been minimized.

Member

matthewd commented May 21, 2018

If we go with this (see parallel discussion in #32945), I think we should also address cover?, and keep all three in the same file.

@utilum

This comment has been minimized.

Contributor

utilum commented May 21, 2018

Thank you both.

I've added cover? and moved all three into a single file, in module RangeCompareWithRange.

Better?

@utilum

This comment has been minimized.

Contributor

utilum commented May 21, 2018

PS: What do you think about moving overlaps? in with the others?

@utilum utilum changed the title from Allow Range#=== on Range to Allow Range#=== and Range#cover? on Range May 21, 2018

guides/source/active_support_core_extensions.md Outdated
@@ -2889,9 +2889,9 @@ As the example depicts, the `:db` format generates a `BETWEEN` SQL clause. That
NOTE: Defined in `active_support/core_ext/range/conversions.rb`.
### `include?`
### `===`, `include?`, and `range?`

This comment has been minimized.

@eugeneius

eugeneius May 21, 2018

Member

range? -> cover?

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Yes! of course.

activesupport/lib/active_support/core_ext/range/include_range.rb Outdated
@@ -1,25 +0,0 @@
# frozen_string_literal: true

This comment has been minimized.

@eugeneius

eugeneius May 21, 2018

Member

The existence of this file is documented, and users may depend on it:

http://guides.rubyonrails.org/v5.2.0/active_support_core_extensions.html#include-questionmark

If we want to remove it, it needs a deprecation cycle first. Here's an example of how that might look:

https://github.com/rails/rails/blob/64c88fb5d2caf3c34742a07394ac68b8377c4936/activesupport/lib/active_support/core_ext/object/to_json.rb

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Thank you. Done.

activesupport/CHANGELOG.md Outdated
ruby/ruby@989e07c features switching `Range#===` to use internal
`r_cover_p` instead of rubyland `include?`. This breaks expected behavior
of `ActiveSupport::CoreExt::Range` documented since at least 8b67a02.

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

This is excellent detail for anyone interested in why the change was necessary, but the changelog should be a high level description of what changed from the user's perspective - it's probably sufficient to say that Range#cover? can now be given a range like Range#=== and Range#include?, and Range#=== works correctly when given a range on Ruby 2.6.

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Cleaned up. I hope I did not go too far by also removing the module name.

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

I think that's the right call - users don't interact with the module directly, they just call the methods that it modifies.

activesupport/lib/active_support/core_ext/range/include_range.rb Outdated
end
end
end
ActiveSupport::Deprecation.warn "You have required `active_support/core_ext/range/include_range`. " \

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

Every other file in Active Support that emits a deprecation requires active_support/deprecation first; we should do the same here.

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Corrected.

guides/source/active_support_core_extensions.md Outdated
The methods `Range#include?` and `Range#===` say whether some value falls between the ends of a given instance:
The methods `Range#===`, `Range#include?` and `Range#cover?` say whether some value falls between the ends of a given instance:

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

http://guides.rubyonrails.org/v5.2.0/api_documentation_guidelines.html#oxford-comma

Please use the Oxford comma ("red, white, and blue", instead of "red, white and blue").

🤓

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Oxford in a comma, it's serious ;)

Allow Range#=== and Range#cover? on Range
ruby/ruby@989e07c features switching `Range#===` to use internal `r_cover_p`
instead of rubyland `include?`. This breaks expected behavior of
`ActiveSupport::CoreExt::Range` documented since at least 8b67a02.

This patch adds overrides on `Range#cover?` and `Range#===` and places all
three in a single module, `CompareWithRange`.

*Requiring core_ext/range/include_range now causes a deprecation warnning*
activesupport/CHANGELOG.md Outdated
* Allow Range#=== and Range#cover? on Range
`Range#cover?` can now accept a range argument like `Range#include` and
`Range===`. `Range#===` works correctly on Ruby 2.6. `Range#include?` is moved

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

There's a missing # in Range=== here.

activesupport/CHANGELOG.md Outdated
To use: `require "active_support/core_ext/compare_range"`.
*Requiring core_ext/range/include_range now causes a deprecation warnning*

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

"warnning" -> "warning"

Can you also add active_support/ to the start of the filename, to match the example above?

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

We're telling the user that requiring this file is deprecated, but not what they need to do instead - it's pretty easy to infer that they should now require active_support/core_ext/compare_range, but ideally we'd explicitly state that.

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Right on.
Sorry about all the typos.

@eugeneius

This comment has been minimized.

Member

eugeneius commented May 22, 2018

What do you think about moving overlaps? in with the others?

I'd recommend leaving it where it is, mostly just to avoid inflicting the mild inconvenience of a deprecation warning on anyone cherry-picking that file on its own.

activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,14 @@
* Allow Range#=== and Range#cover? on Range
`Range#cover?` can now accept a range argument like `Range#include` and

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

Range#include -> Range#include?

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Fixed.

activesupport/CHANGELOG.md Outdated
`Range#===`. `Range#===` works correctly on Ruby 2.6. `Range#include?` is moved
into a new file, with these two methods.
*Requiring active_support_core_ext/range/include_range now causes a deprecation warning*

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

active_support_core_ext -> active_support/core_ext

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

"now causes a deprecation warning" -> "is now deprecated"

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Fixed.

activesupport/CHANGELOG.md Outdated
into a new file, with these two methods.
*Requiring active_support_core_ext/range/include_range now causes a deprecation warning*
*Use `require "active_support/core_ext/compare_range"` instead.*

This comment has been minimized.

@eugeneius

eugeneius May 22, 2018

Member

active_support/core_ext/compare_range -> active_support/core_ext/range/compare_range

This comment has been minimized.

@utilum

utilum May 22, 2018

Contributor

Fixed.

@rafaelfranca rafaelfranca merged commit fa9d01d into rails:master May 22, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@utilum utilum deleted the utilum:range_case_equality branch May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment