Interval improvements: type/range validation, intersection method #114
Conversation
|
Warning Rate limit exceeded@rlskoeser has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request reorganizes the date-handling module by updating documentation, restructuring imports, and modifying the public API. The title and class references in the docs have been updated to emphasize dates, intervals, and calendars. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Undate
participant I as UndateInterval
U->>D: Create new Undate(year, month, day)
alt All components missing
D-->>U: Raise ValueError
else Valid input
D->>D: Validate date components
D-->>U: Return Undate instance
end
U->>D: Call parse(date_string, format)
alt Interval information present
D->>I: Instantiate UndateInterval
I-->>U: Return UndateInterval
else Single date found
D-->>U: Return Undate instance
end
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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:❓ 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.
src/undate/interval.py
Outdated
| def __eq__(self, other) -> bool: | ||
| # consider interval equal if both dates are equal | ||
| return self.earliest == other.earliest and self.latest == other.latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential AttributeError when comparing with non-interval objects.
__eq__ directly compares other.earliest and other.latest without verifying if other is an UndateInterval. Consider returning NotImplemented if type(other) != UndateInterval to avoid runtime errors.
def __eq__(self, other) -> bool:
+ if not isinstance(other, UndateInterval):
+ return NotImplemented
return self.earliest == other.earliest and self.latest == other.latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __eq__(self, other) -> bool: | |
| # consider interval equal if both dates are equal | |
| return self.earliest == other.earliest and self.latest == other.latest | |
| def __eq__(self, other) -> bool: | |
| if not isinstance(other, UndateInterval): | |
| return NotImplemented | |
| # consider interval equal if both dates are equal | |
| return self.earliest == other.earliest and self.latest == other.latest |
| def to_undate(cls, other: object) -> "Undate": | ||
| """Converted arbitrary object to Undate, if possible. Raises TypeError | ||
| if conversion is not possible. | ||
|
|
||
| Currently suppports: | ||
| - :class:`datetime.date` or :class:`datetime.datetime` | ||
|
|
||
| """ | ||
| match other: | ||
| case Undate(): | ||
| return other | ||
| case datetime.date() | datetime.datetime(): | ||
| return Undate(other.year, other.month, other.day) | ||
|
|
||
| case _: | ||
| raise TypeError(f"Conversion from {type(other)} is not supported") | ||
|
|
There was a problem hiding this comment.
Python 3.9 incompatibility: match statement causes SyntaxError.
Your pipeline indicates a syntax error on Python 3.9. To maintain compatibility with 3.9, replace match/case with if/elif logic.
Example refactor:
- match other:
- case Undate():
- return other
- case datetime.date() | datetime.datetime():
- return Undate(other.year, other.month, other.day)
- case _:
- raise TypeError(f"Conversion from {type(other)} is not supported")
+ if isinstance(other, Undate):
+ return other
+ elif isinstance(other, (datetime.date, datetime.datetime)):
+ return Undate(other.year, other.month, other.day)
+ else:
+ raise TypeError(f"Conversion from {type(other)} is not supported")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def to_undate(cls, other: object) -> "Undate": | |
| """Converted arbitrary object to Undate, if possible. Raises TypeError | |
| if conversion is not possible. | |
| Currently suppports: | |
| - :class:`datetime.date` or :class:`datetime.datetime` | |
| """ | |
| match other: | |
| case Undate(): | |
| return other | |
| case datetime.date() | datetime.datetime(): | |
| return Undate(other.year, other.month, other.day) | |
| case _: | |
| raise TypeError(f"Conversion from {type(other)} is not supported") | |
| def to_undate(cls, other: object) -> "Undate": | |
| """Converted arbitrary object to Undate, if possible. Raises TypeError | |
| if conversion is not possible. | |
| Currently suppports: | |
| - :class:`datetime.date` or :class:`datetime.datetime` | |
| """ | |
| if isinstance(other, Undate): | |
| return other | |
| elif isinstance(other, (datetime.date, datetime.datetime)): | |
| return Undate(other.year, other.month, other.day) | |
| else: | |
| raise TypeError(f"Conversion from {type(other)} is not supported") |
🧰 Tools
🪛 GitHub Actions: unit tests
[error] 375-375: SyntaxError: invalid syntax. The 'match' statement is not valid in Python 3.9.
updates in this PR:
UndateIntervaland associated tests into separate files, since the main undate file is getting so longintersectionmethod to compute and return a new interval for the intersection between two intervalsSummary by CodeRabbit
Documentation
New Features
UndateInterval, to represent date ranges with enhanced functionality.Refactor
UndateIntervalclass from the main module, focusing on its dedicated namespace.Tests
Undateclass and removed tests related toUndateInterval.UndateIntervalclass functionality.