Update comparison methods to check for unknown years #145
Conversation
WalkthroughImplements explicit handling for unknown years in Undate: adds public accessors and is_unknown(part), and changes equality, ordering, containment, and duration-related guards when year or parts are unknown. Tests updated to reflect new semantics and compare repr for some ISO8601 unknown-year parses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as caller
participant A as Undate A
participant B as Undate B
Note over A,B: Comparison / containment with possible unknown parts
Caller->>A: request part-known? (year)
Caller->>B: request part-known? (year)
alt either year unknown
note right of Caller #ffd6a5: short-circuit
Caller-->>Caller: return False (eq/ordering/contains) or NotImplemented for type mismatch
else both years known
Caller->>A: compute concrete value
Caller->>B: compute concrete value
Caller-->>Caller: perform normal comparison/containment
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #145 +/- ##
===========================================
+ Coverage 98.72% 98.90% +0.18%
===========================================
Files 38 38
Lines 1956 2010 +54
===========================================
+ Hits 1931 1988 +57
+ Misses 25 22 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/undate/undate.py (1)
393-404: Guard contains against unsupported types.If _comparison_type returns NotImplemented, subsequent attribute access will fail. Return False (item not contained).
Apply:
def __contains__(self, other: object) -> bool: @@ - other = self._comparison_type(other) + other = self._comparison_type(other) + if other is NotImplemented: + return False @@ - if self.unknown_year or other.unknown_year: + if self.unknown_year or other.unknown_year: return False
🧹 Nitpick comments (9)
src/undate/undate.py (4)
303-306: Comment mismatch vs behavior: broaden wording.The check uses known_year (False for unknown or partially known). Update the comment to avoid implying “unknown only.”
Apply:
- # if either date has an unknown year, then not equal - if not self.known_year or not other.known_year: + # if either date does not have a fully known year (unknown or partially known), then not equal + if not self.known_year or not other.known_year: return False
374-385: Clarify boolean precedence for readability.Add parentheses so intent isn’t misread.
Apply:
- if self.unknown_year or isinstance(other, Undate) and other.unknown_year: + if self.unknown_year or (isinstance(other, Undate) and other.unknown_year): return False
386-392: Clarify boolean precedence for readability.Same as above.
Apply:
- if self.unknown_year or isinstance(other, Undate) and other.unknown_year: + if self.unknown_year or (isinstance(other, Undate) and other.unknown_year): return False
463-466: TODO note retained.Acknowledged; behavior matches current tests. Consider documenting this in user docs later.
tests/test_converters/test_iso8601.py (1)
10-13: Avoid coupling test to repr; assert structure instead.repr can change and includes calendar/label. Prefer attribute assertions.
Apply:
- # missing year but month/day known; compare repr string - assert repr(ISO8601DateFormat().parse("--05-03")) == repr( - Undate(month=5, day=3) - ) + # missing year but month/day known; validate structure + parsed = ISO8601DateFormat().parse("--05-03") + assert isinstance(parsed, Undate) + assert parsed.unknown_year is True + assert parsed.month == "05" and parsed.day == "03"tests/test_undate.py (4)
236-238: Fix comment grammar for clarity.“cannot known to be equal” → “cannot be equal” (or “cannot be known to be equal”).
Apply:
- # dates without a known year cannot known to be equal + # dates without a fully known year cannot be equal
263-265: Redundant with Line 236 check (but fine).Both assert the same behavior for unknown-year equality. Keep or dedupe.
519-526: Comment wording is misleading.Header says “known OR partially known” but the next asserts show partially known is not known. Tweak header.
Apply:
- # known OR partially known + # fully known
579-583: Prefer structural assertions over repr here as well.Mirror the converter test pattern.
Apply:
- # dates with unknown year are not equal; compare repr string - assert repr(Undate.parse("--12-31", "ISO8601")) == repr( - Undate(month=12, day=31) - ) + # dates with unknown year are not equal; validate structure + parsed = Undate.parse("--12-31", "ISO8601") + assert isinstance(parsed, Undate) + assert parsed.unknown_year is True + assert parsed.month == "12" and parsed.day == "31"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/undate/undate.py(5 hunks)tests/test_converters/test_iso8601.py(1 hunks)tests/test_undate.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_undate.py (1)
src/undate/undate.py (6)
Undate(51-634)month(480-490)day(493-502)unknown_year(445-447)is_unknown(458-460)parse(257-265)
tests/test_converters/test_iso8601.py (1)
src/undate/undate.py (3)
Undate(51-634)month(480-490)day(493-502)
🔇 Additional comments (8)
src/undate/undate.py (4)
440-443: known_year property looks good.
444-448: unknown_year property addition is clear and consistent with is_unknown().
450-457: is_known semantics align with tests (string numerics normalized to int).
458-461: is_unknown helper is a useful complement; no issues.tests/test_undate.py (4)
313-325: Unknown-year ordering expectations look correct.No issues; matches comparator guards.
379-381: Containment with fully unknown year cases covered well.Good additions.
541-550: unknown_year tests LGTM.
551-568: is_unknown(month/day) tests LGTM and align with current semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/undate/undate.py (1)
377-389: Fix > with unsupported types: avoid boolean ops on NotImplemented.As written,
return not (self < other or self == other)can attemptbool(NotImplemented)whenotheris not convertible, raising TypeError. Convert first and propagate NotImplemented, mirroring lt.Apply:
def __gt__(self, other: object) -> bool: - # define gt ourselves so we can support > comparison with datetime.date, - # but rely on existing less than implementation. - # strictly greater than must rule out equals - - # if either date has a completely unknown year, then we can't compare - # NOTE: this means that gt and lt will both be false when comparing - # with a date with an unknown year... - if self.unknown_year or isinstance(other, Undate) and other.unknown_year: - return False - - return not (self < other or self == other) + # convert first; bail out if not supported + other_u = self._comparison_type(other) + if other_u is NotImplemented: + return NotImplemented + # unknown years are not orderable + if self.unknown_year or other_u.unknown_year: + return False + return not (self < other_u or self == other_u)
♻️ Duplicate comments (1)
src/undate/undate.py (1)
335-341: Good: handle NotImplemented from _comparison_type in lt.Prevents AttributeError and follows Python’s comparison protocol. This addresses the earlier review note.
🧹 Nitpick comments (6)
src/undate/undate.py (3)
342-345: Clarify “unknown year” semantics vs. 'XXXX'.lt gates on unknown_year (absent year), not partially-known 'XXXX'. That’s probably intentional, but it differs from calculate_earliest_latest treating 'XXXX' as None for range calc. Please confirm this distinction is desired and, if so, document it in unknown_year/is_unknown docstrings to avoid confusion.
466-469: Validate part input for is_unknown.Guard against typos and unsupported parts to avoid silent False.
Apply:
def is_unknown(self, part: str) -> bool: - """Check if a part of the date (year, month, day) is completely unknown.""" - return self.initial_values.get(part) is None + """Check if a part of the date (year, month, day) is completely unknown.""" + if part not in ("year", "month", "day"): + raise ValueError("part must be one of 'year', 'month', or 'day'") + return self.initial_values.get(part) is None
584-613: Guard against empty possible_max_days to avoid KeyError.Edge precisions (future additions like decade/century) would hit
.pop()on an empty set.Apply:
- # if there is more than one possible value for number of days - # due to range including lear year / non-leap year, return an uncertain delta - if len(possible_max_days) > 1: - return UnDelta(*possible_max_days) - return Timedelta(possible_max_days.pop()) + # if there is more than one possible value for number of days + # due to range including leap vs non-leap years, return an uncertain delta + if len(possible_max_days) > 1: + return UnDelta(*possible_max_days) + if len(possible_max_days) == 1: + return Timedelta(possible_max_days.pop()) + # defensive: unsupported precision path + raise NotImplementedError( + f"Duration not supported for precision {self.precision.name} when year is not fully known" + )tests/test_undate.py (3)
192-198: Test manipulates internals; consider avoiding private state hacks.Directly mutating
initial_valuesandprecisioncouples the test to internals. If feasible, prefer constructing an instance that naturally exercises the branch, or skip this niche scenario.
526-533: Tweak comment wording for known_year tests.“known OR partially known” is misleading since partially known years are asserted False. Suggest: “Known year only (partially known is not known).”
Apply:
- # known OR partially known + # Known year only (partially known is not known)
328-330: Add symmetric tests for gt/le NotImplemented.To prevent regressions from boolean ops on NotImplemented, add:
def test_lt_notimplemented(self): # unsupported type should bail out and return NotImplemented assert Undate(2022).__lt__("foo") == NotImplemented + + def test_gt_le_notimplemented(self): + assert Undate(2022).__gt__("foo") == NotImplemented + assert Undate(2022).__le__("foo") == NotImplemented
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/undate/undate.py(6 hunks)tests/test_undate.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_undate.py (1)
src/undate/undate.py (7)
Undate(51-645)year(477-485)month(488-498)day(501-510)unknown_year(453-455)is_unknown(466-468)parse(257-265)
src/undate/undate.py (1)
src/undate/date.py (2)
UnDelta(125-169)Timedelta(11-31)
🔇 Additional comments (10)
src/undate/undate.py (3)
303-306: Equality correctly rejects unknown-year operands.This directly satisfies #143 by making two unknown-year Undates unequal. Looks good.
401-405: Good: contains now returns NotImplemented on unsupported types.Matches Python’s protocol and your new tests.
576-583: Duration fast-path for fully known dates is clear.The inclusive ONE_DAY adjustment is correct and consistent with tests.
tests/test_undate.py (7)
237-239: Equality behavior for unknown-year is correct per #143.Asserting non-equality for identical month/day without year matches the new semantics.
314-326: Comprehensive gt/lt checks for unknown-year cases.Great coverage to ensure all four relations are False.
328-330: Nice: explicit NotImplemented assertion for lt with bad type.This guards protocol correctness.
363-365: Good: contains NotImplemented path is exercised.Matches the updated implementation.
385-388: Containment with fully unknown year is well covered.These cases ensure “unknown” does not imply containment.
548-575: Nice additions for unknown_year and is_unknown(part).These clearly define the API semantics in tests.
586-589: repr-based comparison for unknown-year parses is pragmatic.Avoids equality semantics while still validating parse fidelity.
| def __le__(self, other: object) -> bool: | ||
| # if either date has a completely unknown year, then we can't compare | ||
| if self.unknown_year or isinstance(other, Undate) and other.unknown_year: | ||
| return False | ||
|
|
||
| return self == other or self < other |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix <= with unsupported types: avoid boolean ops on NotImplemented.
Same concern as gt; convert first and propagate NotImplemented.
Apply:
def __le__(self, other: object) -> bool:
- # if either date has a completely unknown year, then we can't compare
- if self.unknown_year or isinstance(other, Undate) and other.unknown_year:
- return False
-
- return self == other or self < other
+ other_u = self._comparison_type(other)
+ if other_u is NotImplemented:
+ return NotImplemented
+ if self.unknown_year or other_u.unknown_year:
+ return False
+ return self == other_u or self < other_u📝 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 __le__(self, other: object) -> bool: | |
| # if either date has a completely unknown year, then we can't compare | |
| if self.unknown_year or isinstance(other, Undate) and other.unknown_year: | |
| return False | |
| return self == other or self < other | |
| def __le__(self, other: object) -> bool: | |
| other_u = self._comparison_type(other) | |
| if other_u is NotImplemented: | |
| return NotImplemented | |
| if self.unknown_year or other_u.unknown_year: | |
| return False | |
| return self == other_u or self < other_u |
🤖 Prompt for AI Agents
In src/undate/undate.py around lines 390 to 395, the __le__ implementation
currently performs boolean ops on other without converting it and may evaluate
expressions when other is NotImplemented; first attempt to convert other to an
Undate (call the same conversion helper used in __gt__/__lt__), and if that
conversion returns NotImplemented, return NotImplemented immediately; then
perform the unknown_year checks against the converted value and finally return
self == other or self < other — this prevents boolean ops involving
NotImplemented and mirrors the pattern used for other rich comparisons.
resolves #143 - comparison methods had logic for partially known years but not for fully unknown years
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes
Documentation
Tests