Fix beginless and endless range comparisons

[AlexMooney]

Motivation / Background

This Pull Request has been created because some endless and beginless range comparisons raise RangeError due to ActiveSupport::CompareWithRange

Detail

This Pull Request changes the CompareWithRange#=== and CompareWithRange#include? methods to support begin and endless ranges on the left and right hand side of the comparison.

Additional information

POR:

> (1..) === (2..)
=> false
> (..1) === (..2)
=> false

Rails 7.1.3:

> (1..) === (2..)
RangeError: cannot get the last element of endless range
from …/activesupport-7.1.3/lib/active_support/core_ext/range/compare_range.rb:23:in `last'
> (..1) === (..2)
RangeError: cannot get the first element of beginless range
from …/activesupport-7.1.3/lib/active_support/core_ext/range/compare_range.rb:24:in `first'

This PR:

> (1..) === (2..)
=> true
> (2..) === (1..)
=> false
> (..1) === (..2)
=> false
> (..2) === (..1)
=> true

Checklist

Before submitting the PR make sure the following are checked:

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

[skipkayhil]

Both methods have this comment which should be changed:

# The given range must be fully bounded, with both start and end.

---

Thinking out loud: It seems like include? ends up very similar if not the same as cover?, are there any differences left?

Edit: introduced in 99c6482, I wonder if we should just recommend people use cover? now 🤔

[yawboakye]

@AlexMooney i've looked at the code over and over again and i think it could be written in a clearer way. a lot of things get in the way of comprehension. i recommend that you separate the endless/beginless range concern, and handle with a private method.

[AlexMooney]

Both methods have this comment which should be changed:

# The given range must be fully bounded, with both start and end.

Thanks for catching that! I've removed it.

Thinking out loud: It seems like include? ends up very similar if not the same as cover?, are there any differences left?

I don't think there are any differences in numerical values, but in non-numeric ranges you can get different answers:

> ('a'..'d').=== 'cc'
true
> ('a'..'d').include? 'cc'
false

Edit: introduced in 99c6482, I wonder if we should just recommend people use cover? now 🤔

The reason I ended up here is this extension messing with ranges makes some spec like expect(array_of_beginless_ranges).to contain_exactly(expected) blow up. It might be better to remove these overrides entirely in some breaking Rails release, though!

@AlexMooney i've looked at the code over and over again and i think it could be written in a clearer way. a lot of things get in the way of comprehension. i recommend that you separate the endless/beginless range concern, and handle with a private method.

@yawboakye I've rewritten it to use smaller private methods so that I could name the concepts involved. Let me know if you like this way better. I'll squish the PR back into 1 commit after the code looks acceptable.

[yawboakye]

one more (and final) refactoring. from where i sit, both === and include? seem to have identical implementations, so i recommend we make one the alias of the other.

[AlexMooney]

one more (and final) refactoring. from where i sit, both === and include? seem to have identical implementations, so i recommend we make one the alias of the other.

That is almost but not entirely so. The super that they yield is subtly different from one another. See the string range from my previous comment for an example of the difference.

[yawboakye]

That is almost but not entirely so. The super that they yield is subtly different from one another. See the string range from my previous comment for an example of the difference.

good point 👍! we can still move the range check condition outside of compare_with_range though so that we’re only ever entering that method when we’re certain other is a range.

[AlexMooney]

I've separated the ::Range check method from the compare_with_range and also added tests to demonstrate the difference between === and .include?.

[yawboakye]

looks good to me cc @skipkayhil

[AlexMooney]

@skipkayhil can I get another review, please?

[AlexMooney]

@skipkayhil nudge to take a look at the PR again, please.