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

Rename Range#overlaps? to Range#overlap? #48565

Merged
merged 1 commit into from
Jun 24, 2023

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Jun 23, 2023

Motivation

The method name Range#overlaps? uses a 3rd person verb which is inconsistent with e.g. Range#include? and Range#cover?.

Detail

This pull request renames Range#overlaps? to Range#overlap? and adds an alias for backwards compatibility (similar to String#starts_with?).

Checklist

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Rename for consistency with #include? and #cover?
@guilleiguaran guilleiguaran merged commit 672376c into rails:main Jun 24, 2023
9 checks passed
@yahonda
Copy link
Member

yahonda commented Jun 24, 2023

overlaps? is one of the public Rails API, even if it is going to be renamed, it needs to have a deprecation period.

https://api.rubyonrails.org/classes/Range.html#method-i-overlaps-3F

@guilleiguaran
Copy link
Member

@yahonda the plan is to leave both methods for 7.1 and deprecate overlaps? in the next major version (8.0)

@yahonda
Copy link
Member

yahonda commented Jun 25, 2023

Understood. Thanks.

@c960657 c960657 deleted the range-overlap branch June 25, 2023 07:44
yahonda added a commit to yahonda/rails that referenced this pull request Sep 19, 2023
This commit uses Ruby 3.3 `Range#overlap?` that has been added to Ruby via ruby/ruby#8242 .
Rails 7.1 renames `Range#overlaps?` to `Range#overlap?` via rails#48565 ,
This commit is not feasible to backport because there is no `Range#overlap?` in Rails 7.0.z

This commit addresses the CI faiilure at https://buildkite.com/rails/rails/builds/99745#018a9ea8-82f0-40a6-90c3-cdaa6dabebab/1092-1095
because without this commit, it shows `warning: method redefined; discarding old overlap?`.
```ruby
$ ruby -v
ruby 3.3.0dev (2023-09-16T05:57:19Z master e9b503f1bb) [x86_64-linux]
$ RAILS_STRICT_WARNING=true bundle exec ruby -w -Itest test/core_ext/range_ext_test.rb
/home/yahonda/src/github.com/rails/rails/activesupport/lib/active_support/core_ext/range/overlap.rb:7: warning: method redefined; discarding old overlap?
Running 46 tests in a single process (parallelization threshold is 50)
Run options: --seed 583

\# Running:

..............................................

Finished in 0.011672s, 3940.9670 runs/s, 4883.3722 assertions/s.
46 runs, 57 assertions, 0 failures, 0 errors, 0 skips
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants