Interval logic improvements: type/range validation, intersection method, contains/in functionality #114
Conversation
WalkthroughThis update enhances the date handling functionality by modifying both the Changes
Sequence Diagram(s)sequenceDiagram
participant A as Interval A
participant B as Interval B
participant I as Intersection Method
A->>I: Call intersection(B)
I->>A: Retrieve A.earliest & A.latest
I->>B: Retrieve B.earliest & B.latest
I->>I: Compute max(earliest) and min(latest)
alt Intersection exists
I->>A: Return new Interval (overlap)
else No intersection
I->>A: Return None
end
sequenceDiagram
participant C as Client
participant U as UndateInterval Constructor
participant T as Undate.to_undate
C->>U: Provide input dates
U->>T: Convert input using to_undate
T-->>U: Return converted Undate values (or raise error)
U->>C: Construct and return UndateInterval
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (1)tests/test_interval.py (2)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/undate/interval.py (3)
39-53: Consider consolidating date-conversion logic into a helper.Lines 39-53 perform identical checks and conversions for
earliestandlatest. Repetition can be reduced by creating a small helper function (e.g._to_undate_if_possible) to keep the code DRY and improve maintainability.def __init__(...): ... - if earliest and not isinstance(earliest, Undate): - if isinstance(earliest, datetime.date): - earliest = Undate.from_datetime_date(earliest) - else: - raise ValueError(...) - - if latest and not isinstance(latest, Undate): - if isinstance(latest, datetime.date): - latest = Undate.from_datetime_date(latest) - else: - raise ValueError(...) + earliest = self._to_undate_if_possible(earliest) + latest = self._to_undate_if_possible(latest)
62-65: Use consistent open-ended notation in__str__.Currently,
Nonefor earliest becomes".."whileNonefor latest becomes"", which risks confusion. Consider using".."for both ends when they areNoneto more clearly indicate open-ended intervals.
126-149: Add coverage for edge cases in intersection tests.The intersection logic gracefully returns
Nonefor invalid intervals, but you might consider explicitly testing cases where:
- One or both intervals have
Nonefor earliest/latest- Overlapping intervals share only a boundary day
tests/test_interval.py (1)
147-178: Consider adding more edge cases for intersection tests.The intersection tests are good but could be enhanced with additional edge cases:
- Intervals with identical start/end dates
- Intervals with partial date information (e.g., only month/day)
- Intervals with unknown components
Example test cases to add:
def test_intersection_edge_cases(self): # Identical intervals same_date = UndateInterval(Undate(2000, 1, 1), Undate(2000, 1, 1)) assert same_date.intersection(same_date) == same_date # Partial date information month_only = UndateInterval(Undate(2000, 1), Undate(2000, 2)) day_only = UndateInterval(Undate(2000, 1, 15), Undate(2000, 1, 20)) assert month_only.intersection(day_only) == day_only # Unknown components unknown_month = UndateInterval(Undate(2000, None, 1), Undate(2000, None, 31)) assert unknown_month.intersection(month_only) is None # or handle differently
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/undate/core.rst(1 hunks)src/undate/__init__.py(1 hunks)src/undate/converters/calendars/hebrew/converter.py(1 hunks)src/undate/converters/calendars/hebrew/transformer.py(1 hunks)src/undate/converters/calendars/hijri/converter.py(1 hunks)src/undate/converters/calendars/hijri/transformer.py(1 hunks)src/undate/converters/edtf/converter.py(1 hunks)src/undate/converters/edtf/transformer.py(1 hunks)src/undate/converters/iso8601.py(1 hunks)src/undate/interval.py(1 hunks)src/undate/undate.py(4 hunks)tests/test_converters/edtf/test_edtf_transformer.py(1 hunks)tests/test_converters/test_edtf.py(2 hunks)tests/test_converters/test_iso8601.py(1 hunks)tests/test_interval.py(1 hunks)tests/test_undate.py(3 hunks)
✅ Files skipped from review due to trivial changes (8)
- tests/test_converters/test_iso8601.py
- src/undate/converters/edtf/converter.py
- src/undate/converters/edtf/transformer.py
- src/undate/converters/calendars/hijri/transformer.py
- src/undate/converters/iso8601.py
- tests/test_converters/edtf/test_edtf_transformer.py
- src/undate/converters/calendars/hebrew/converter.py
- src/undate/converters/calendars/hijri/converter.py
🔇 Additional comments (16)
src/undate/interval.py (2)
54-57: Verify whether zero-length intervals are intentionally disallowed.The check
if latest and earliest and latest <= earliest:raises an error for an interval with the same start and end date. Confirm if zero-length intervals (i.e. earliest == latest) should be supported for scenarios where an interval is a single day.
97-97: ClarifyNotImplementedfor open-ended intervals induration.When
self.earliestorself.latestisNone, the method returnsNotImplemented. This makes sense if open-ended durations are not supported. Documenting or raising a more explicit error (e.g., aValueError) may improve understanding.tests/test_undate.py (3)
5-5: Import changes look correct.Importing
UndateIntervalandCalendaralongsideUndatealigns with the refactored module structure.
133-136: Confirm partial year handling for "19??".Line 135 raises a
ValueErrorfor"19??". Ensure that the parser logic treats “??” differently from “XX” so no unintended overlap or confusion arises for partial year patterns.
137-138: Correct exception testing for no-argument initialization.Verifying a
ValueErrorwhen all arguments areNoneis consistent with the new requirement that at least one date component must be specified.src/undate/undate.py (3)
1-9: Forward reference usage forUndateIntervalis properly handled.Using the
TYPE_CHECKINGguard with a forward import ofUndateIntervalmaintains type safety without introducing circular imports at runtime.
75-77: Enforcing at least one date component is a good constraint.These lines add a clear validation check, preventing incomplete
Undateobjects with no year, month, or day. This improves overall data integrity.
229-229: Updatedparsesignature for type clarity.Switching to
-> Union["Undate", UndateInterval]improves clarity for callers, reflecting that parsing might return an interval.src/undate/__init__.py (1)
4-5: LGTM! Well-organized module structure.The changes improve the module organization by:
- Moving
UndateIntervalto a dedicated module- Making
Calendara first-class citizen in the public APIAlso applies to: 7-7
src/undate/converters/calendars/hebrew/transformer.py (1)
3-3: LGTM! Simplified import path.The import statement has been simplified to use the public API directly, which is cleaner and more maintainable.
tests/test_converters/test_edtf.py (2)
3-3: LGTM! Updated import path.The import statement has been updated to use the public API directly.
66-69:Details
❓ Verification inconclusive
Verify the removal of empty Undate support.
The commented code indicates that empty Undate instances are no longer supported. This could be a breaking change for existing users.
Let's check for any remaining usage of empty Undate instances:
🏁 Script executed:
#!/bin/bash # Search for potential empty Undate instantiations rg -l "Undate\(\s*\)" --type py # Search for any documentation mentioning empty Undate support rg -l "empty.*Undate|Undate.*empty" --type rst --type mdLength of output: 148
Empty Undate Removal Verification – Manual Check Recommended
The search confirms that the empty Undate instantiation in tests/test_converters/test_edtf.py is now commented out (i.e. removed). Additionally, no documentation files (RST/MD) reference support for an “empty” Undate. Note that some usages of
Undate()still exist (for example in tests/test_undate.py); please verify that these invocations are not inadvertently instantiating an empty Undate (and thus relying on removed behavior).tests/test_interval.py (1)
1-146: LGTM! Comprehensive test coverage for UndateInterval.The test suite thoroughly covers:
- Type validation and conversion
- Error handling
- Edge cases and special cases
- String representation and formatting
- Duration calculations
docs/undate/core.rst (3)
4-4: Title Update Reflects Expanded Scope
The updated title "dates, intervals, and calendar" effectively communicates the expanded focus of the module. Please double-check that this new wording aligns with the rest of your documentation and module naming conventions.
10-11: Documenting the New Calendar Class
Introducing the.. autoclass:: undate.undate.Calendarblock enhances clarity regarding the new public API. Verify that the documented class members are current and that the source code forCalendaraccurately reflects this configuration.
13-13: Inclusion of the UndateInterval Class from Its New Namespace
The addition of.. autoclass:: undate.interval.UndateIntervalcorrectly mirrors the module reorganization by reflecting its new location. Ensure that references in other parts of the documentation and module import paths are updated accordingly to prevent any linkage inconsistencies.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/undate/interval.py (3)
26-28: Consider removing or implementing the optional precision field.There is a
TODOcomment about adding an optional precision/length/size field. If this feature is still desired, you could implement it or clarify whether it’s needed. If it remains a future enhancement, consider creating a tracking issue or removing the comment.
37-42: Useraise ... from errfor clearer exception context.When a
TypeErroris caught, static analysis tools (e.g. Ruff B904) recommend usingraise ValueError(...) from errorraise ValueError(...) from Noneto preserve the original exception context.Below is an example fix:
try: earliest = Undate.to_undate(earliest) except TypeError as err: - raise ValueError( - f"earliest date {earliest} cannot be converted to Undate" - ) + raise ValueError( + f"earliest date {earliest} cannot be converted to Undate" + ) from err🧰 Tools
🪛 Ruff (0.8.2)
40-42: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
45-47: Applyraise ... from errfor latest date conversion too.Same as above: use
raise ... from errorraise ... from Nonefor clearer traceback.try: latest = Undate.to_undate(latest) except TypeError as err: - raise ValueError(f"latest date {latest} cannot be converted to Undate") + raise ValueError(f"latest date {latest} cannot be converted to Undate") from err🧰 Tools
🪛 Ruff (0.8.2)
47-47: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/test_undate.py (1)
167-172: Revisit commented-out code related to no date info.The code is commented out due to no longer supporting initialization without date info. If the requirement has changed permanently, consider removing these lines. Otherwise, open an issue tracking potential future reintroduction.
src/undate/undate.py (1)
257-260: RaisingNotImplementedvs. returning it.Consider returning
NotImplementedfrom_comparison_typeso normal Python fallback comparison behavior occurs, rather than raising aNotImplementedErrorat higher levels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/undate/interval.py(1 hunks)src/undate/undate.py(7 hunks)tests/test_undate.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/undate/interval.py
40-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Actions: unit tests
src/undate/undate.py
[error] 375-375: SyntaxError: invalid syntax. The 'match' statement is not valid in Python 3.9.
🔇 Additional comments (10)
src/undate/interval.py (2)
50-51: Clarify whether inclusive single-day intervals are allowed.Currently, the check raises an error when
latest <= earliest. If you want to support intervals where earliest == latest, adjust the condition tolatest < earliest. Otherwise, ensure this strict rule is intentional.
121-144: Validate partial intervals in intersection logic.
intersectionmerges earliest and latest from two intervals. If one interval is unbounded on one side, logic allows reuse of the other’s date. Verify if partial intervals (e.g., earliest is None) are handled consistently.tests/test_undate.py (5)
1-1: Import looks good.Importing
dateanddatetimefromdatetimeis standard practice.
5-5: Check ifUndateIntervalis needed in this file.
UndateIntervalis imported at line 5. Confirm that you’re using it (e.g., for parse tests). Otherwise, remove the import to keep dependencies minimal.
135-137: Approved test coverage for invalid year format.This test ensures
Undate("19??")is invalid. No concerns here.
145-146: Test method name is descriptive and clear.
test_to_undateclearly indicates the behavior under test forUndate.to_undate(). Good naming choice.
150-154: Validate the datetime conversion more thoroughly.The test checks conversion from
datetime.now(). Consider also verifying boundary behaviors such as earliest or partial times (if relevant).src/undate/undate.py (3)
1-1: Postponed evaluation of annotations is helpful for forward references.
from __future__ import annotationsis a good practice for type-hinting clarity.
78-80: Good validation for requiring at least one date component.Raising a
ValueErrorhere ensures no entirely emptyUndateinstances. This is a solid user constraint.
230-232: Flexible parse return type is beneficial.Returning
Union["Undate", UndateInterval]fromparseaccommodates intervals. This helps manage uncertain date ranges seamlessly.
rlskoeser
force-pushed
the
feature/intersection
branch
from
ce44a52 to
a4f2e7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/undate/interval.py (2)
35-47: Add exception context usingraise ... from errfor better traceability.The error handling in the constructor correctly catches
TypeErrorexceptions during conversion but loses the original error context. Usingraise ... from errwould preserve this context, making it easier to debug issues.try: earliest = Undate.to_undate(earliest) -except TypeError: +except TypeError as err: - raise ValueError(f"earliest date {earliest} cannot be converted to Undate") + raise ValueError(f"earliest date {earliest} cannot be converted to Undate") from err try: latest = Undate.to_undate(latest) -except TypeError: +except TypeError as err: - raise ValueError(f"latest date {latest} cannot be converted to Undate") + raise ValueError(f"latest date {latest} cannot be converted to Undate") from err🧰 Tools
🪛 Ruff (0.8.2)
40-42: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
47-47: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
124-146: Well-implemented intersection method with good error handling.The intersection implementation is clean and handles the edge cases well. It correctly determines the overlap between intervals and properly manages cases where
earliestorlatestmight beNone.A couple of suggestions for improvement:
- The docstring could clarify what "no overlap" means exactly (e.g., when the resulting interval would be invalid).
- The error handling silently returns
NoneonValueError, which might hide unexpected errors.Consider enhancing the docstring and adding more specific error handling:
def intersection(self, other: "UndateInterval") -> Optional["UndateInterval"]: """Determine the intersection or overlap between two :class:`UndateInterval` objects and return a new interval, or None if no overlap. + + Returns None if: + - The intervals don't overlap (latest of one is earlier than earliest of the other) + - The resulting interval would be invalid (latest <= earliest) """ try: # when both values are defined, return the inner bounds; # if not, return whichever is not None, or None earliest = ( max(self.earliest, other.earliest) if self.earliest and other.earliest else self.earliest or other.earliest ) latest = ( min(self.latest, other.latest) if self.latest and other.latest else self.latest or other.latest ) # if this results in an invalid interval, initialization # will throw an exception return UndateInterval(earliest, latest) - except ValueError: + except ValueError as err: + # Only catch the specific ValueError we expect (invalid interval) + if "invalid interval" in str(err): + return None + # Re-raise unexpected ValueErrors + raise return Nonetests/test_undate.py (2)
145-157: Good test coverage for the newto_undatemethod.The tests for the new
to_undatemethod cover the essential cases - conversion fromdate,datetime, and handling unsupported types. Consider adding more edge cases to strengthen test coverage.Add tests for additional edge cases, such as:
def test_to_undate_edge_cases(self): # Test with an Undate instance (should return the same object) original = Undate(2024, 3, 15) converted = Undate.to_undate(original) assert converted is original # Should be the same object, not just equal # Test with None with pytest.raises(TypeError): Undate.to_undate(None) # Test with invalid types with pytest.raises(TypeError): Undate.to_undate(123) with pytest.raises(TypeError): Undate.to_undate([2024, 3, 15])
171-175: Consider removing commented-out code.Instead of commenting out code that's no longer supported, consider removing it entirely. Commented-out code can become stale and confusing as the codebase evolves.
Replace the commented code with a more explicit test that verifies the new behavior:
-# NOTE: no longer supported to inistalize undate with no date information -# force method to hit conditional for date precision -# some_century = Undate() -# some_century.precision = DatePrecision.CENTURY -# assert some_century.year is None # Test that initializing without parameters raises an error def test_empty_undate_raises(self): with pytest.raises(ValueError, match="At least one of year, month, or day must be specified"): Undate()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/unit_tests.yml(1 hunks)pyproject.toml(1 hunks)src/undate/interval.py(3 hunks)src/undate/undate.py(5 hunks)tests/test_converters/test_edtf.py(1 hunks)tests/test_interval.py(2 hunks)tests/test_undate.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/unit_tests.yml
- tests/test_interval.py
- pyproject.toml
- tests/test_converters/test_edtf.py
🧰 Additional context used
🪛 Ruff (0.8.2)
src/undate/interval.py
40-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (5)
src/undate/interval.py (1)
76-80: Improved type handling in equality check.Great job implementing the type check and returning
NotImplementedfor non-UndateInterval types. This follows best practices for implementing comparison operators.tests/test_undate.py (1)
137-138: Good validation for required parameters.Adding a test for the new validation that ensures at least one date component is specified is a good practice.
src/undate/undate.py (3)
77-80: Good validation for required parameters.Adding a check to ensure at least one date component is specified improves the robustness of the class. This ensures that users can't create meaningless
Undateinstances.
251-262: Improved type conversion with class method approach.Changing
_comparison_typeto a class method and using the newto_undatemethod for conversion is a good refactoring. This centralizes the conversion logic and makes it more maintainable.
366-382:Details
✅ Verification successful
New
to_undatemethod uses Python 3.10+ features.The implementation of
to_undateuses the match-case pattern matching syntax introduced in Python 3.10. This is a clean implementation but requires dropping support for earlier Python versions.The PR summary mentions dropping support for Python 3.9, but let's confirm this is clearly documented in any project metadata files:
Also, consider expanding the docstring to clarify all supported types:
@classmethod def to_undate(cls, other: object) -> "Undate": """Converted arbitrary object to Undate, if possible. Raises TypeError if conversion is not supported. Currently suppports: - :class:`datetime.date` or :class:`datetime.datetime` + - :class:`Undate` (returns the same instance) + + Raises: + TypeError: If the provided object cannot be converted to an Undate instance. + + Returns: + Undate: A converted Undate instance. + + Example: + >>> from datetime import date + >>> today = date.today() + >>> undate = Undate.to_undate(today) + >>> isinstance(undate, Undate) + True """
🏁 Script executed:
#!/bin/bash # Check Python version requirements in setup.py or pyproject.toml if [ -f setup.py ]; then echo "Checking Python version requirement in setup.py:" grep -A 5 "python_requires" setup.py fi if [ -f pyproject.toml ]; then echo "Checking Python version requirement in pyproject.toml:" grep -A 5 "requires-python" pyproject.toml fi # Check for any mentions of minimum Python version in README or docs echo "Checking README for Python version mentions:" if [ -f README.md ]; then grep -i "python.*version\|version.*python\|python.*3\.10" README.md fi # Check CI config for Python version mentions echo "Checking CI config for Python versions:" find .github -name "*.yml" -exec grep -l "python" {} \; | xargs grep -i "python-version"Length of output: 1269
Python 3.10+ Requirement Confirmed & Docstring Enhancement Suggested
The newto_undatemethod correctly leverages match-case syntax, now that our minimum Python version is confirmed as ≥3.10 (as seen inpyproject.tomland our CI configurations). The implementation is clean; however, it would be beneficial to update the docstring to explicitly mention all supported types and the possible exception. For example:@classmethod def to_undate(cls, other: object) -> "Undate": """Converted arbitrary object to Undate, if possible. Raises TypeError if conversion is not supported. Currently suppports: - :class:`datetime.date` or :class:`datetime.datetime` + - :class:`Undate` (returns the same instance) + + Raises: + TypeError: If the provided object cannot be converted to an Undate instance. + + Returns: + Undate: A converted Undate instance. + + Example: + >>> from datetime import date + >>> today = date.today() + >>> undate = Undate.to_undate(today) + >>> isinstance(undate, Undate) + True """
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_interval.py (2)
85-90: Typo in comment on line 89.There's a minor typo in the comment: "NotIplemented" should be "NotImplemented".
- # returns NotIplemented if comparison with this type is not supported + # returns NotImplemented if comparison with this type is not supported
185-231: Line 210 has a misleading syntax - add an assertion.The static analyzer flagged line 210 as having a "pointless comparison." Since this is inside a
pytest.raisesblock, you should specify what you're testing with anassertstatement:- "nineteen-eighty-four" in century20th + assert "nineteen-eighty-four" in century20thThe rest of the containment tests are comprehensive and well-structured, covering all the important scenarios.
🧰 Tools
🪛 Ruff (0.8.2)
210-210: Pointless comparison. Did you mean to assign a value? Otherwise, prepend
assertor remove it.(B015)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/undate/interval.py(3 hunks)tests/test_interval.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_interval.py
210-210: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
🔇 Additional comments (5)
tests/test_interval.py (1)
153-184: The intersection method implementation looks robust and well-tested.This test method thoroughly covers critical scenarios for interval intersection including:
- No intersection between intervals
- Complete containment
- Partial overlaps
- Half-open intervals
The test cases provide good coverage for the new functionality.
src/undate/interval.py (4)
35-49: Enhanced type conversion and error handling.The improved constructor now leverages
Undate.to_undatefor consistent type conversion with clear error messages. The error handling properly preserves the original exception cause withfrom err, making debugging easier.
77-81: Type check in equality method prevents potential runtime errors.The added type check correctly implements Python's equality protocol by returning
NotImplementedfor non-UndateIntervaltypes. This prevents potentialAttributeErrorexceptions when comparing with incompatible types.
126-159: Well-implemented containment method with comprehensive type handling.The
__contains__implementation properly handles:
- Comparison with other interval objects
- Conversion of dates to Undate for comparison
- Type error handling for unsupported types
- Special handling for open-ended intervals
The implementation follows Python's object model conventions and supports the natural syntax of
date in intervalorsmaller_interval in larger_interval.
161-183: Clean and efficient intersection implementation.The intersection method elegantly:
- Handles the max/min calculations for interval bounds
- Properly manages None values for open intervals
- Returns None for non-overlapping intervals
- Uses exception handling to simplify validation logic
The implementation is concise yet handles all edge cases effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_interval.py (2)
153-184: Well-designed intersection tests with good coverage.The tests for intersection functionality are comprehensive, covering key scenarios including no intersection, complete containment, partial overlap, and half-open intervals. The test cases demonstrate that the new intersection method handles various edge cases correctly.
One suggestion for completeness: consider adding a test case for the intersection of two identical intervals to verify that the result equals both intervals.
# Test identical intervals identical_intervals = UndateInterval(Undate(1950), Undate(1960)) assert identical_intervals.intersection(identical_intervals) == identical_intervals
185-231: Thorough containment tests covering multiple scenarios.Great job testing various containment scenarios with clear assertions. The tests cover well-defined cases (interval containing another interval or a date), edge cases (half-open intervals), and error handling for unsupported types.
I noticed that lines 189-191 test that an interval doesn't contain itself (
assert interval not in interval). This behavior is intentional but might be surprising to users familiar with set theory (where a set is a subset of itself). Consider adding a comment in the implementation to explain this design decision.The test is missing explicit boundary tests. Consider adding tests for dates exactly at the start and end boundaries to clarify the inclusion/exclusion behavior:
# Test boundary cases y1990 = Undate(1990) # Start of decade1990s y1999 = Undate(1999) # End of decade1990s assert y1990 in decade1990s # Is start date included? assert y1999 in decade1990s # Is end date included?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_interval.py(2 hunks)
🔇 Additional comments (1)
tests/test_interval.py (1)
85-90: LGTM! Well-structured test for type check handling.This is a clean implementation that correctly tests if the equality operator returns
NotImplementedwhen compared with unsupported types, which follows Python's best practices for equality comparisons.
rlskoeser
changed the title
updates in this PR:
intersectionmethod to compute and return a new interval for the intersection between two intervals__contains__method to check if one interval or date is contained by another intervalSummary by CodeRabbit
Summary by CodeRabbit