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

union, intersection and overlaps? for Ranges #15106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CTC97
Copy link

@CTC97 CTC97 commented Oct 21, 2024

Apologies for the earlier PR from my forked master branch.

Based on the request from @jgaskins. Behaviors below:

a = 1..5
b = 3..7
a.union(b)        # => 1..7
a.intersection(b) # => 3..5

c = 1..10
d = 1...15
c.union(d)        # => 1...15
c.intersection(d) # => 1..10

e = 1...10
f = 1..15
e.union(f)        # => 1..15
e.intersection(f) # => 1...10

g = 1..5
h = 10..20
g.union(h)        # => 0..0
g.intersection(h) # => 0..0

(1...10).union(1..10)        # => 1..10
(1...10).intersection(1..10) # => 1...10

(1...10).union(10..20) # => 1..20

(1..5).union(5..10) # => 1..10

(1...10).intersection(7..12) # => 7...10
(2..10).intersection(0..8)   # => 2..8

(1...10).overlaps?(10..11)   # => false
(1..10).overlaps?(5..9) # => true

Fixes #14487
Fixes #14488


P.S., very excited to be making a small contribution to a lovely language.

Looking forward to review!

@jgaskins
Copy link
Contributor

This looks like a great start. A couple areas of focus:

  • Ranges can be nil on either end
  • It would be awesome to see some tests — this functionality can be super confusing and, honestly, I wouldn't be able to tell whether any implementation was correct when all edge cases are considered

# ```
def union(other : Range)
if self.end < other.begin || other.end < self.begin
return 0..0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can get away with hardcoding a Range(Int32, Int32) specifically. My own use case for this functionality is actually Range(Time, Time), so I'd get compile-time errors with this return value.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, and thank you for the quick review!

I'll get on these changes asap! ✌️⭐

@Blacksmoke16 Blacksmoke16 changed the title union, intersection and overlaps? for Ranges union, intersection and overlaps? for Ranges Oct 21, 2024
@Blacksmoke16
Copy link
Member

Some specs would be a good idea as well 👍.

@jgaskins
Copy link
Contributor

Tests and specs? In this economy?

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.

Union and intersection of Ranges Add a method to Range to detect overlap/intersection with another Range
4 participants