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

Add ranges.any_gaps #185

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Add ranges.any_gaps #185

merged 5 commits into from
Jan 13, 2025

Conversation

Peter554
Copy link
Contributor

@Peter554 Peter554 commented Jan 13, 2025

This PR adds a ranges.any_gaps utility function.

This function is very simple - all work is delegated to a RangeSet. So arguably this function is not needed at all. I think it's still a useful addition though, since this check is commonly used and this utility function makes it slightly clearer and less likely to make an error.

A benchmark is added, however the existing implementation was found to be pretty optimal. I suggest leaving the benchmark to help guard against regressions (I'm interested to set up codespeed for this, see here).

To give us even more confidence that the order of the input
ranges does not matter.
@Peter554 Peter554 self-assigned this Jan 13, 2025
@Peter554 Peter554 marked this pull request as ready for review January 13, 2025 09:33
@@ -589,6 +589,17 @@ def test_timestamps_sorted(self, period):


class TestAnyOverlapping:
def test_does_not_modify_ranges(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably have been part of #184

@@ -624,6 +624,7 @@ def test_does_not_modify_ranges(self):
)
def test_returns_true_if_and_ranges_overlap(self, ranges_):
assert ranges.any_overlapping(ranges_)
assert ranges.any_overlapping(reversed(ranges_))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably have been part of #184

Copy link

@leamingrad leamingrad left a comment

Choose a reason for hiding this comment

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

LGTM!

@Peter554 Peter554 merged commit 12bee09 into main Jan 13, 2025
1 check passed
@Peter554 Peter554 deleted the ranges.any-gaps branch January 13, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants