get dt index for time unit #15
Conversation
WalkthroughAdds index↔date mapping and subscripting for time-unit kinds via an IndexableMixin and SlicedProxy; implements get_index_for_date/get_date_from_index for Decade and other kinds, updates Timeunit iteration/truncation and tests in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Kind as TimeunitKind (meta)
participant IndexMix as IndexableMixin
participant Unit as Timeunit
Tester->>Kind: get_index_for_date(dt)
Kind-->>Tester: index
Tester->>Kind: get_date_from_index(index)
Kind-->>Tester: dt0
Tester->>Kind: Kind[index] or Kind[start:stop]
Kind->>IndexMix: __getitem__(index_or_slice)
alt integer index
IndexMix->>Kind: _from_index(index)
Kind->>Kind: get_date_from_index(index)
Kind-->>Unit: Timeunit(dt0)
Unit-->>Tester: Timeunit instance
else slice
IndexMix-->>Tester: SlicedProxy (iterable)
note right of IndexMix #E6F0FF: SlicedProxy iterates using Kind._from_index
end
Note right of Kind: __len__/__iter__ bounded by first_date / last_date
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)timetest.py (1)
🪛 Ruff (0.13.3)timetest.py151-151: Pointless comparison. Did you mean to assign a value? Otherwise, prepend (B015) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (2)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
193-195: Remove unnecessarymax()wrapper.For valid months (1-12),
(dt.month - 1) // 3always yields 0-3, somax(..., 0)is redundant. Themax()call would only matter for invalid months (≤ 0), but the code should not be processing such dates.Apply this diff:
- return 4 * (dt.year - date.min.year) + max((dt.month - 1) // 3, 0) + return 4 * (dt.year - date.min.year) + (dt.month - 1) // 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
timetest.py(2 hunks)unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
timetest.py (1)
unit_of_time/__init__.py (8)
get_index_for_date(139-140)get_index_for_date(178-179)get_index_for_date(194-195)get_index_for_date(227-228)get_index_for_date(248-250)get_index_for_date(268-269)next(355-356)previous(289-290)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (6)
unit_of_time/__init__.py (6)
139-140: LGTM!The default implementation returning
Noneallows subclasses to optionally implement index computation. The static analysis hint about the unuseddtparameter is a false positive for this base implementation.
177-179: LGTM!The year index calculation is correct and efficiently computes the offset from
date.min.year.
191-191: LGTM!The formula
(dt.month+2)//3is mathematically equivalent to(dt.month-1)//3 + 1and correctly maps months 1-3 to Q1, 4-6 to Q2, 7-9 to Q3, and 10-12 to Q4.
225-227: LGTM!The month index calculation is correct and efficiently computes a sequential index from
date.min.
246-249: LGTM!The week index calculation is correct. The comment noting that
date.min.weekday() == 0(Monday) confirms that the division by 7 properly aligns weeks.
266-268: LGTM!The day index calculation is straightforward and correct, computing the number of days since
date.min.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
197-199: Remove unnecessarymax()wrapper.For valid dates,
(dt.month - 1) // 3always yields 0–3, so themax(..., 0)wrapper is redundant.Apply this diff:
@classmethod def get_index_for_date(cls, dt): - return 4 * (dt.year - date.min.year) + max((dt.month - 1) // 3, 0) + return 4 * (dt.year - date.min.year) + (dt.month - 1) // 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (7)
unit_of_time/__init__.py (7)
139-140: LGTM! Base implementation is appropriate.The default
Nonereturn is a reasonable base behavior for time unit kinds that don't implement indexing. The static analysis warning about the unuseddtparameter is a false positive—it's expected in this polymorphic base method.
173-175: LGTM! Efficient truncation implementation.Direct date construction is more efficient than the default formatter-based approach.
181-183: LGTM!Straightforward year index calculation.
195-195: LGTM!The quarter calculation
(dt.month+2)//3correctly produces 1-based quarters (1-4) for display.
229-231: LGTM!Correct month index calculation.
270-272: LGTM!Straightforward day index calculation.
250-253: Truncate the date before calculating the week index.The current implementation can return different indices for dates within the same week. For example, if
dtis a Friday,(dt - date.min).days // 7may yield a different result than for the Monday of that same week, violating the expectation that all dates in a time unit share the same index.Apply this diff to truncate first:
@classmethod def get_index_for_date(cls, dt): # date.min has weekday() == 0 - return (dt - date.min).days // 7 + return (cls.truncate(dt) - date.min).days // 7Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
197-199: Consider removing the unnecessarymax(..., 0)wrapper.The expression
(dt.month - 1) // 3already produces values in the range [0, 3] for valid months (1-12), so themax(..., 0)wrapper is redundant.- return 4 * (dt.year - date.min.year) + max((dt.month - 1) // 3, 0) + return 4 * (dt.year - date.min.year) + (dt.month - 1) // 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
timetest.py(3 hunks)unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
unit_of_time/__init__.py (1)
timetest.py (2)
get_index_for_date(25-26)truncate(12-22)
timetest.py (1)
unit_of_time/__init__.py (7)
get_index_for_date(139-140)get_index_for_date(182-183)get_index_for_date(198-199)get_index_for_date(230-231)get_index_for_date(251-253)get_index_for_date(271-272)next(357-358)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
275-275: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (11)
timetest.py (3)
24-26: LGTM!The implementation correctly computes the decade index. Using
dt.year // 10is the natural choice for decade boundaries (0-9, 10-19, etc.) and correctly returns 0 fordate.min(year 1).
106-113: LGTM!These assertions effectively verify two critical properties:
- Monotonicity: Each time unit's index is exactly 1 less than its successor's index
- Intra-unit consistency: All dates within a time unit share the same index
212-212: LGTM!This boundary condition test ensures all time unit kinds correctly anchor their index to zero at
date.min, which is essential for consistent indexing across the hierarchy.unit_of_time/__init__.py (8)
139-140: Base implementation returns None as expected.The static analysis warning about unused
dtis a false positive—the parameter is required to match the signature expected by derived classes. The base implementation intentionally returnsNonefor time unit kinds that don't override this method.
173-175: LGTM!This override is a good optimization. The base
truncateusesdatetime.strptime(cls.to_str(dt), cls.formatter).date(), which is less efficient than directly constructingdate(dt.year, 1, 1).
181-183: LGTM!Correctly computes the year index relative to
date.min.year, ensuring the index is 0 atdate.minand increments by 1 for each subsequent year.
195-195: LGTM!The quarter calculation
(dt.month+2)//3correctly maps months to quarters: January-March→Q1, April-June→Q2, July-September→Q3, October-December→Q4.
229-231: LGTM!Correctly computes the month index using 12 months per year offset plus the current month (0-indexed), ensuring the index is 0 at
date.minand increments by 1 for each month.
250-253: LGTM!The implementation is correct, and the comment noting that
date.min.weekday() == 0(Monday) provides useful context for understanding the week alignment.
270-272: LGTM!The simplest and most direct implementation—the day index is exactly the number of days since
date.min.
279-279: LGTM!The signature is consistent with the base class and other time unit implementations.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
timetest.py(3 hunks)unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
timetest.py (1)
unit_of_time/__init__.py (13)
get_index_for_date(139-140)get_index_for_date(190-191)get_index_for_date(210-211)get_index_for_date(248-249)get_index_for_date(275-277)get_index_for_date(299-300)next(389-390)get_date_from_index(142-143)get_date_from_index(194-195)get_date_from_index(214-217)get_date_from_index(252-255)get_date_from_index(280-281)get_date_from_index(303-304)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
142-142: Unused method argument: dt
(ARG002)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (5)
unit_of_time/__init__.py (4)
139-148: LGTM!The indexing API is well-designed. Base methods returning
Noneallow subclasses to opt into indexing support, and__getitem__provides a clean interface for bracket-notation access.
181-195: LGTM!The
truncateoverride is more efficient than the base implementation, and the index methods correctly implement bidirectional mapping between years and indices.
274-281: LGTM!The Week indexing implementation correctly uses day offsets from
date.minand properly handles the roundtrip conversion.
298-312: LGTM!The Day indexing implementation correctly uses day offsets from
date.min, and the roundtrip conversion is accurate.timetest.py (1)
106-114: Excellent test coverage for the indexing API.The assertions comprehensively validate:
- Sequential indexing (current index = next index - 1)
- Roundtrip consistency (date → index → date)
- Bracket notation access (
kind[idx])- Index stability within a time unit
These tests will catch the bugs in
Quarter.get_date_from_indexandMonth.get_date_from_indexwhen executed.
| @classmethod | ||
| def get_index_for_date(cls, dt): | ||
| return dt.year // 10 |
There was a problem hiding this comment.
Inconsistent indexing pattern with other time units.
Decade returns the absolute decade number (dt.year // 10), while all other units return offsets from date.min. For example:
- Year:
dt.year - date.min.year - Month:
12 * (dt.year - date.min.year) + dt.month - 1 - Day:
(dt - date.min).days
This works for the current date.min.year=1 but breaks the design pattern. If date.min.year changes, Decade would be incorrect.
Apply this diff to align with the offset-based pattern:
@classmethod
def get_index_for_date(cls, dt):
- return dt.year // 10
+ return (dt.year - date.min.year) // 10Note: You'll also need to implement get_date_from_index for Decade to support the full indexing API:
@classmethod
def get_date_from_index(cls, idx):
return date((idx * 10) + date.min.year, 1, 1)🤖 Prompt for AI Agents
In timetest.py around lines 24-26, get_index_for_date currently returns an
absolute decade (dt.year // 10) which is inconsistent with other units; change
it to return the offset-based decade index: (dt.year - date.min.year) // 10.
Also add a companion classmethod get_date_from_index that returns date((idx *
10) + date.min.year, 1, 1) so the decade indexing API is complete and consistent
with date.min offsets.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
timetest.py (1)
24-31: Fix decade index mapping and year-0 bug; align with other kinds
- get_date_from_index(0) builds year 0 (invalid). Must offset by date.min.year.
- Decade index should be offset from date.min like Year/Month/Week/Day for consistency and future-proofing.
Apply this diff:
@classmethod def get_index_for_date(cls, dt): - return dt.year // 10 + return (dt.year - date.min.year) // 10 @classmethod def get_date_from_index(cls, idx): - return date(10 * idx, 1, 1) + return date(date.min.year + 10 * idx, 1, 1)Additionally, make truncate consistent with the same origin (outside the changed hunk):
@classmethod def truncate(cls, dt): base = date.min.year return date(base + 10 * ((dt.year - base) // 10), 1, 1)Based on relevant_code_snippets for Year/Month/Week/Day.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
timetest.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
timetest.py (1)
unit_of_time/__init__.py (15)
get_index_for_date(139-140)get_index_for_date(190-191)get_index_for_date(210-211)get_index_for_date(248-249)get_index_for_date(275-277)get_index_for_date(299-300)last_day(102-112)get_date_from_index(142-143)get_date_from_index(194-195)get_date_from_index(214-217)get_date_from_index(252-255)get_date_from_index(280-281)get_date_from_index(303-304)previous(323-324)next(389-390)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (1)
timetest.py (1)
217-217: LGTM: consistent origin assertionAsserting kind.get_index_for_date(date.min) == 0 aligns all kinds on a common origin.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
145-158: Add fenced code languages to satisfy markdownlint.The new code fences lack a language tag, triggering MD040. Please label them (for example,
python) so the docs lint passes.Here’s an example patch:
-``` -Week.get_index_for_date(date(1958, 3, 25)) # 102123 -Week.get_date_from_index(102123) # date(1958, 3, 24) -``` +```python +Week.get_index_for_date(date(1958, 3, 25)) # 102123 +Week.get_date_from_index(102123) # date(1958, 3, 24) +``` -``` -Week[102123] # Week(date(1958, 3, 24)) -``` +```python +Week[102123] # Week(date(1958, 3, 24)) +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
147-147: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
unit_of_time/__init__.py (1)
213-217: Critical: Quarter index-to-date conversion still broken.This is the same issue identified in the previous review. For
idx=0:
yy = (0-1)//4 = -1qq = 0 - 4*(-1) = 4date(-1, 13, 1)→ Invalid monthThe fix from the previous review still applies.
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
139-148: LGTM! Base indexing API is well-structured.The default implementations returning
Noneand the__getitem__delegation are appropriate. The approach allows subclasses to override as needed.Optional: Consider extracting the error message to a constant for consistency with the codebase style, though the current inline approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
get_index_for_date(25-26)get_date_from_index(29-30)truncate(12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
142-142: Unused method argument: dt
(ARG002)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (6)
unit_of_time/__init__.py (6)
181-195: LGTM! Year indexing is correctly implemented.The index-to-date conversion is the proper inverse of date-to-index. For
date.min(year 1): index = 0, and reverse mapping correctly returnsdate(1, 1, 1).
207-207: LGTM! Quarter string formatting is correct and more concise.The formula
(dt.month+2)//3correctly maps months to quarters (1-3→Q1, 4-6→Q2, etc.) and is equivalent to the previous(dt.month-1)//3 + 1but cleaner.
247-255: LGTM! Month indexing is correctly implemented.The index-to-date conversion properly handles
idx=0(maps todate(1, 1, 1)) and is the correct inverse of date-to-index. This implementation differs from the broken version mentioned in the past review and works correctly.
274-281: LGTM! Week indexing is correctly implemented.The implementation correctly counts weeks from
date.minand the inverse mapping is straightforward. The comment noting thatdate.minhasweekday() == 0(Monday) is helpful context.
298-304: LGTM! Day indexing is correctly implemented.The straightforward day-counting from
date.minand its inverse are correct.
310-312: LGTM! Signature aligns with classmethod decorator.Changing the first parameter from
selftoclscorrectly reflects the@classmethoddecorator, making it consistent with other time unit classes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
unit_of_time/__init__.py (1)
213-217: Fix the index-to-date conversion for Quarter.Despite the past review comment indicating this was addressed, the bug persists. The formula fails for
idx=0andidx=4:
idx=0 (should be Q1 of year 1):
yy = (0 - 1) // 4 = -1qq = 0 - 4 * (-1) = 4date(0, 13, 1)→ ValueError: year 0 and month 13 are invalid!idx=4 (should be Q1 of year 2):
yy = (4 - 1) // 4 = 0qq = 4 - 4 * 0 = 4date(1, 13, 1)→ ValueError: month 13 is invalid!The issue stems from mixing 1-based and 0-based indexing.
Apply this diff:
@classmethod def get_date_from_index(cls, idx): - yy = (idx - 1) // 4 - qq = idx - 4 * yy - return date(yy+1, 3 * qq + 1, 1) + yy = idx // 4 + date.min.year + qq = idx % 4 + return date(yy, 3 * qq + 1, 1)
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
145-148: LGTM!The
__getitem__implementation correctly enables index-based access (e.g.,Week[102123]). The logic properly delegates toget_date_from_indexand wraps the result in aTimeunitinstance.If you prefer, you could address the Ruff TRY003 hint by defining a custom exception class instead of using a long message string:
class IndexLookupError(TypeError): """Raised when an item cannot be used for index lookup.""" pass def __getitem__(cls, item): if isinstance(item, int): return cls(cls.get_date_from_index(item)) raise IndexLookupError(f"Cannot lookup an index for {item}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
get_index_for_date(25-26)get_date_from_index(29-30)truncate(12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
142-142: Unused method argument: dt
(ARG002)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (6)
unit_of_time/__init__.py (6)
189-195: LGTM!The index-to-date conversion for
Yearis correct. The implementation properly usesdate.min.yearas the base, ensuring that index 0 maps to the first year and round-trips work correctly.
207-207: LGTM!The new quarter formatting formula
(dt.month+2)//3is mathematically equivalent to the old(dt.month-1)//3+1but more elegant.
247-255: LGTM!The index-to-date conversion for
Monthis correct. Unlike the similar issue flagged forQuarter, theMonthimplementation properly handles the base year offset withyy + 1, ensuring index 0 correctly maps to January ofdate.min.year(year 1).
274-281: LGTM!The
Weekindex methods are correct and straightforward. The implementation properly leveragesdate.minas the base and uses timedelta arithmetic for reliable conversions. The comment aboutdate.min.weekday() == 0is helpful context.
298-304: LGTM!The
Dayindex methods are correct and follow the same straightforward pattern asWeek, usingdate.minas the base and timedelta arithmetic for conversions.
311-312: LGTM!The signature change to make
_nexta classmethod alignsDaywith the base class signature (line 114) and is consistent with other time unit implementations. Since the method doesn't require instance state, this is the correct design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_of_time/__init__.py (2)
214-217: Consider usingdate.min.yearfor consistency.The current implementation assumes
date.min.year == 1(which is correct in Python), but for consistency withYear,Week, andDayimplementations, consider explicitly usingdate.min.year:@classmethod def get_date_from_index(cls, idx): - yy = idx // 4 + yy = idx // 4 + date.min.year qq = idx - 4 * yy - return date(yy + 1, 3 * qq + 1, 1) + return date(yy, 3 * qq + 1, 1)However, note that this requires adjusting the
yynow represents an absolute year rather than an offset:@classmethod def get_date_from_index(cls, idx): yy = idx // 4 + date.min.year - qq = idx - 4 * yy + qq = idx % 4 return date(yy, 3 * qq + 1, 1)
252-255: Consider usingdate.min.yearfor consistency.Similar to the Quarter implementation, this code assumes
date.min.year == 1. For consistency with other time unit implementations, consider:@classmethod def get_date_from_index(cls, idx): - yy = idx // 12 + yy = idx // 12 + date.min.year mm = (idx % 12) + 1 - return date(yy + 1, mm, 1) + return date(yy, mm, 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_of_time/__init__.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
get_index_for_date(25-26)get_date_from_index(29-30)truncate(12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
139-139: Unused method argument: dt
(ARG002)
142-142: Unused method argument: dt
(ARG002)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (4)
unit_of_time/__init__.py (4)
139-148: LGTM: Index-based access foundation.The base implementations correctly return
Noneto be overridden by subclasses, and__getitem__appropriately delegates toget_date_from_indexfor integer indices.Note: The static analysis warnings about unused arguments in lines 139 and 142 are false positives—these base methods are intentionally designed to be overridden.
181-195: LGTM: Year index conversion is correct.The index formulas correctly implement a 0-based offset from
date.min.year, ensuring that index 0 maps to January 1 ofdate.min.yearand the round-trip conversion is accurate.
274-281: LGTM: Week index conversion is correct.The implementation correctly calculates weeks as day offsets divided by 7. The comment noting that
date.minhasweekday() == 0provides helpful context for the calculation.
298-311: LGTM: Day index conversion and signature update are correct.The index formulas correctly use day offsets from
date.min, and the_nextmethod signature correctly reflects its@classmethoddecorator with theclsparameter.Note: The static analysis warning about the unused
curparameter in line 307 is a false positive—the parameter maintains signature consistency with other_inner_shiftimplementations across time unit kinds.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
unit_of_time/__init__.py (1)
159-160: Fix parameter name for consistency.The parameter should be
idx(notdt) to match the subclass implementations and accurately reflect that this method converts an index to a date.Apply this diff:
- def get_date_from_index(cls, dt): + def get_date_from_index(cls, idx): return None
🧹 Nitpick comments (2)
unit_of_time/__init__.py (2)
226-229: Prefer explicitdate.min.yearfor consistency.While
yy + 1is technically correct (sincedate.min.yearis always 1), usingdate.min.yearexplicitly would align with the Year implementation and make the code more maintainable.Apply this diff:
def get_date_from_index(cls, idx): yy = idx // 4 qq = idx - 4 * yy - return date(yy + 1, 3 * qq + 1, 1) + return date(yy + date.min.year, 3 * qq + 1, 1)
264-267: Prefer explicitdate.min.yearfor consistency.Similar to Quarter, using
date.min.yearexplicitly would improve clarity and align with the Year implementation.Apply this diff:
def get_date_from_index(cls, idx): yy = idx // 12 mm = (idx % 12) + 1 - return date(yy + 1, mm, 1) + return date(yy + date.min.year, mm, 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_of_time/__init__.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(29-30)get_index_for_date(25-26)truncate(12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
156-156: Unused method argument: dt
(ARG002)
159-159: Unused method argument: dt
(ARG002)
319-319: Unused class method argument: cur
(ARG003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (8)
unit_of_time/__init__.py (8)
18-27: Well-structured indexing mixin.The implementation correctly handles both single indices and slices by leveraging range normalization, which properly handles negative indices and slice objects. The delegation to
_from_indexprovides good separation of concerns.
29-29: Indexing integration is well-designed.The multiple inheritance order is correct (IndexableMixin before type), and the methods properly integrate indexing support at the metaclass level. The
__len__calculation correctly accounts for 0-based indexing by adding 1 to the maximum index.Also applies to: 43-44, 67-68
193-207: Clean Year indexing implementation.The index↔date conversion is correct and explicit, using
date.min.yearfor clarity. The round-trip property holds:idx=0maps todate.min, and indices correctly represent years offset from the minimum.
219-219: Quarter calculation formula is correct.The
(dt.month+2)//3formula accurately maps months to quarters 1-4.
286-293: Week indexing is correctly implemented.The calculations properly handle week boundaries, and the comment accurately notes that
date.minfalls on a Monday (weekday 0), ensuring consistency with thetruncatemethod.
310-316: Day indexing is straightforward and correct.The simple day offset calculations properly map indices to dates, with
idx=0correctly representingdate.min.
323-323: _next signature change aligns with base class usage.Changing from an instance method to a classmethod is correct, as the base class invokes it as
cls._next(dt)on line 151.
327-327: Timeunit indexing provides intuitive day access.Inheriting from
IndexableMixinand implementing_from_indexallows natural indexing into days within a time unit (e.g.,month[0]returns the first day). This complements the metaclass-level indexing nicely.Also applies to: 380-381
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
README.md (5)
112-116: Addpythonlanguage identifier to this fenced block.This snippet is Python, so prefix the opening fence with ```python for proper highlighting.
121-123: Addpythonlanguage identifier here as well.Like the previous block, mark the fence as ```python to satisfy markdownlint.
127-129: Annotate the fence withpython.Please open this block with ```python for consistency.
133-135: Specifypythonfor the slice example.Prefix the fence with ```python so the code is highlighted correctly.
142-145: Add thepythonlanguage tag here.Mark the fenced block as ```python to resolve the lint warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
110-110: Spaces inside code span elements
(MD038, no-space-in-code)
112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
142-142: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
README.md
Outdated
| The `Day`, `Week`, `Month, etc. classes have `.get_index_for_date(..)` and `.get_date_from_index(..)` methods, which allow to determine how many days, weeks, months, quarters and years are between `date.min` and the date given, and convert this back to a date. For example: | ||
|
|
||
| ``` | ||
| Week.get_index_for_date(date(1958, 3, 25)) # 102123 | ||
| Week.get_date_from_index(102123) # date(1958, 3, 24) | ||
| ``` | ||
|
|
||
| so 1958-03-25 is the 102'123 week since 0001-01-01, and that week starts the 24<sup>th</sup> of March, 1958. | ||
|
|
There was a problem hiding this comment.
Fix inline code formatting and class name typo.
The inline code span currently reads Month, etc., which swallows the trailing text and leaves TimUnit misspelled. Please close the Month code span before the comma and spell the class name as Timeunit to match the API.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
110-110: Spaces inside code span elements
(MD038, no-space-in-code)
112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In README.md around lines 110 to 118 the inline code span for "Month" is not
closed before the comma and the class name "TimUnit" is misspelled; fix the
markdown by closing the inline code span for Month (e.g. `Month`) before the
comma, and correct the class name to `Timeunit` (or use `Timeunit` in backticks
wherever referenced) so the inline code formatting and API name are consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
unit_of_time/__init__.py (3)
564-575: Convert datetime to date before computing the index.The subtraction
(dt - date.min).dayswill fail ifdtis adatetimeobject. The docstring (lines 570-571) states the method accepts bothdateanddatetime, so conversion is required.Apply this diff:
@classmethod def get_index_for_date(cls, dt): - # date.min has weekday() == 0 """ Compute the zero-based week index of a given date relative to date.min (weeks start on Monday). Parameters: dt (datetime.date | datetime.datetime): The date to index; when a datetime is provided, its date component is used. Returns: int: Number of whole weeks between date.min (which is a Monday) and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days // 7
615-625: Convert datetime to date before computing the index.Same issue as
Week.get_index_for_date: the subtraction will fail ifdtis adatetime. The docstring states the method accepts both types.Apply this diff:
@classmethod def get_index_for_date(cls, dt): """ Compute the day-based index of a date relative to date.min. Parameters: dt (datetime.date | datetime.datetime): The date to convert into an index. Returns: int: Number of days between `date.min` and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days
275-285: Rename parameter fromdttoidxfor clarity.The parameter is named
dtbut represents an integer index, not a date. This is confusing and inconsistent with subclass implementations and the method's purpose.Apply this diff:
- def get_date_from_index(cls, dt): + def get_date_from_index(cls, idx): """ Map an index value for this time unit kind to its corresponding start date. Parameters: - dt (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). + idx (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). Returns: - date_or_none (datetime.date | None): The start date corresponding to `dt`, or `None` when the kind does not provide a mapping. + date_or_none (datetime.date | None): The start date corresponding to `idx`, or `None` when the kind does not provide a mapping. """ return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
timetest.py(4 hunks)unit_of_time/__init__.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
timetest.py (1)
unit_of_time/__init__.py (23)
get_index_for_date(261-273)get_index_for_date(364-374)get_index_for_date(422-433)get_index_for_date(498-508)get_index_for_date(564-575)get_index_for_date(615-625)get_date_from_index(275-285)get_date_from_index(377-387)get_date_from_index(436-448)get_date_from_index(511-523)get_date_from_index(578-588)get_date_from_index(628-638)last_day(215-225)truncate(297-307)truncate(338-348)truncate(451-461)truncate(591-603)Year(333-402)Quarter(405-475)Month(478-540)Week(543-607)Day(610-667)next(771-772)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(38-48)get_index_for_date(25-35)truncate(12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
79-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
261-261: Unused method argument: dt
(ARG002)
275-275: Unused method argument: dt
(ARG002)
390-390: Unused class method argument: cur
(ARG003)
417-417: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
549-549: Unused class method argument: cur
(ARG003)
641-641: Unused class method argument: cur
(ARG003)
844-844: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run tests
🔇 Additional comments (13)
unit_of_time/__init__.py (10)
5-6: LGTM!The
ONE_DAYconstant improves code readability and maintainability.
18-28: LGTM!The docstring clearly documents the function's behavior.
31-73: LGTM!The
IndexableMixinandSlicedProxyclasses provide clean, Pythonic support for indexing and slicing. The implementation correctly handles both integer indices and slice objects, delegating to_from_indexfor the actual item retrieval.
76-158: LGTM!The enhanced
TimeunitKindMetawith index-based operations (_from_index,__len__, etc.) provides a solid foundation for the indexing infrastructure. The lazy initialization of the registry and multiplier is efficient.
249-259: LGTM!The
to_strmethod correctly handles the year formatting edge case by replacing%Ywith a zero-padded 4-digit year.
261-273: Rename parameter fromdttoidxfor clarity.The parameter name
dtis misleading because this method maps a date/datetime to an index, not an index to a date. The parameter should be nameddtand the return type should beint | None. The current implementation is correct, but the unused parameter warning from Ruff is a false positive for this base method.Actually, reviewing the docstring at lines 268-271, the parameter is correctly documented as
dt (datetime.date | datetime.datetime). The Ruff warning is a false positive because base implementations of abstract methods naturally don't use their parameters.
337-402: LGTM!The
Yearclass implementation with index-based operations is correct. The offset-based indexing (dt.year - date.min.year) ensures robustness.
408-468: LGTM!The
Quarterclass index operations are now correct after previous fixes.
482-540: LGTM!The
Monthclass index operations are now correct after previous fixes.
670-756: LGTM!The
Timeunitclass with index-based day iteration is well-implemented. The_from_indexmethod correctly returns dates offset from the unit's start.timetest.py (3)
128-136: LGTM!The index validation tests correctly verify the round-trip consistency between
get_index_for_dateandget_date_from_index, as well as the indexing behavior for all dates within each time unit.
196-198: LGTM!The repr tests validate the string representations for both the kind itself and sliced proxies.
239-241: LGTM!The tests correctly validate that
get_index_for_date(date.min)returns 0 for all kinds and that slicing works as expected.
timetest.py
Outdated
| @classmethod | ||
| def get_index_for_date(cls, dt): | ||
| """ | ||
| Return the zero-based decade index for the given date. | ||
|
|
||
| Parameters: | ||
| dt (date or datetime): The date for which to compute the decade index. | ||
|
|
||
| Returns: | ||
| int: The decade index equal to the calendar year divided by 10 using integer division (year // 10). | ||
| """ | ||
| return dt.year // 10 | ||
|
|
||
| @classmethod | ||
| def get_date_from_index(cls, idx): | ||
| """ | ||
| Return the start date (January 1) of the decade represented by the given index. | ||
|
|
||
| Parameters: | ||
| idx (int): Decade index; the corresponding year is 10 * idx. | ||
|
|
||
| Returns: | ||
| datetime.date: January 1 of the year 10 * idx. | ||
| """ | ||
| return date(10 * idx, 1, 1) |
There was a problem hiding this comment.
Fix the index calculation to be consistent with other time units.
The current implementation has two issues:
-
Inconsistent with other units:
Decade.get_index_for_datereturns the absolute decade number (dt.year // 10), while all other time units return offsets fromdate.min. For example:Year:dt.year - date.min.yearMonth:12 * (dt.year - date.min.year) + dt.month - 1
-
Broken for index 0:
get_date_from_index(0)returnsdate(0, 1, 1), which raisesValueErrorbecause year 0 doesn't exist in the Gregorian calendar. While the current tests don't hit this case (START_DATE is year 902), any code callingDecade.get_date_from_index(0)orDecade[0]will fail.
Apply this diff to fix both issues:
@classmethod
def get_index_for_date(cls, dt):
"""
Return the zero-based decade index for the given date.
Parameters:
dt (date or datetime): The date for which to compute the decade index.
Returns:
- int: The decade index equal to the calendar year divided by 10 using integer division (year // 10).
+ int: The decade index relative to date.min, computed as (year - date.min.year) // 10.
"""
- return dt.year // 10
+ return (dt.year - date.min.year) // 10
@classmethod
def get_date_from_index(cls, idx):
"""
Return the start date (January 1) of the decade represented by the given index.
Parameters:
- idx (int): Decade index; the corresponding year is 10 * idx.
+ idx (int): Decade index relative to date.min; the corresponding year is (10 * idx) + date.min.year.
Returns:
- datetime.date: January 1 of the year 10 * idx.
+ datetime.date: January 1 of the year (10 * idx) + date.min.year.
"""
- return date(10 * idx, 1, 1)
+ return date((10 * idx) + date.min.year, 1, 1)This ensures:
Decade.get_index_for_date(date.min)returns 0 (for year 1:(1 - 1) // 10 = 0)Decade.get_date_from_index(0)returnsdate(1, 1, 1)(valid)- The pattern is consistent with
Year,Quarter,Month,Week, andDay
📝 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.
| @classmethod | |
| def get_index_for_date(cls, dt): | |
| """ | |
| Return the zero-based decade index for the given date. | |
| Parameters: | |
| dt (date or datetime): The date for which to compute the decade index. | |
| Returns: | |
| int: The decade index equal to the calendar year divided by 10 using integer division (year // 10). | |
| """ | |
| return dt.year // 10 | |
| @classmethod | |
| def get_date_from_index(cls, idx): | |
| """ | |
| Return the start date (January 1) of the decade represented by the given index. | |
| Parameters: | |
| idx (int): Decade index; the corresponding year is 10 * idx. | |
| Returns: | |
| datetime.date: January 1 of the year 10 * idx. | |
| """ | |
| return date(10 * idx, 1, 1) | |
| @classmethod | |
| def get_index_for_date(cls, dt): | |
| """ | |
| Return the zero-based decade index for the given date. | |
| Parameters: | |
| dt (date or datetime): The date for which to compute the decade index. | |
| Returns: | |
| int: The decade index relative to date.min, computed as (year - date.min.year) // 10. | |
| """ | |
| return (dt.year - date.min.year) // 10 | |
| @classmethod | |
| def get_date_from_index(cls, idx): | |
| """ | |
| Return the start date (January 1) of the decade represented by the given index. | |
| Parameters: | |
| idx (int): Decade index relative to date.min; the corresponding year is (10 * idx) + date.min.year. | |
| Returns: | |
| datetime.date: January 1 of the year (10 * idx) + date.min.year. | |
| """ | |
| return date((10 * idx) + date.min.year, 1, 1) |
🤖 Prompt for AI Agents
In timetest.py around lines 24 to 48, Decade.get_index_for_date and
get_date_from_index are inconsistent with other units and produce invalid year
0; change get_index_for_date to compute the offset from date.min (i.e. (dt.year
- date.min.year) // 10) so date.min maps to index 0, and change
get_date_from_index to construct the decade start using that offset added back
to date.min.year (i.e. year = 10 * idx + date.min.year) so
get_date_from_index(0) returns a valid date(1,1,1).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
timetest.py (2)
129-138: Avoid O(N²) test runtime by running the inner loop once per time unit.Iterating all dates in
tufor everydtexplodes runtime, especially forDecade(~3,650 days). Run the inner loop only whendtis the unit's first day.Apply this diff:
- for idx2, dt2 in enumerate(tu): - self.assertEqual(idx, kind.get_index_for_date(dt2)) - self.assertEqual(dt2, tu[idx2]) + if dt == tu.first_date: + for idx2, dt2 in enumerate(tu): + self.assertEqual(idx, kind.get_index_for_date(dt2)) + self.assertEqual(dt2, tu[idx2])
25-49: Fix the index calculation to be consistent with other time units.The current implementation has two critical issues that were flagged in previous reviews but remain unfixed:
Inconsistent with other units:
Decade.get_index_for_datereturns the absolute decade number (dt.year // 10), while all other time units return offsets fromdate.min. For example:
Year:dt.year - date.min.yearMonth:12 * (dt.year - date.min.year) + dt.month - 1Broken for index 0:
get_date_from_index(0)returnsdate(0, 1, 1), which raisesValueErrorbecause year 0 doesn't exist in the Gregorian calendar. While the current tests don't hit this case (START_DATE is year 902), any code callingDecade.get_date_from_index(0)orDecade[0]will fail.Apply this diff to fix both issues:
@classmethod def get_index_for_date(cls, dt): """ Return the zero-based decade index for the given date. Parameters: dt (date or datetime): The date for which to compute the decade index. Returns: - int: The decade index equal to the calendar year divided by 10 using integer division (year // 10). + int: The decade index relative to date.min, computed as (year - date.min.year) // 10. """ - return dt.year // 10 + return (dt.year - date.min.year) // 10 @classmethod def get_date_from_index(cls, idx): """ Return the start date (January 1) of the decade represented by the given index. Parameters: - idx (int): Decade index; the corresponding year is 10 * idx. + idx (int): Decade index relative to date.min; the corresponding year is (10 * idx) + date.min.year. Returns: - datetime.date: January 1 of the year 10 * idx. + datetime.date: January 1 of the year (10 * idx) + date.min.year. """ - return date(10 * idx, 1, 1) + return date((10 * idx) + date.min.year, 1, 1)This ensures:
Decade.get_index_for_date(date.min)returns 0 (for year 1:(1 - 1) // 10 = 0)Decade.get_date_from_index(0)returnsdate(1, 1, 1)(valid)- The pattern is consistent with
Year,Quarter,Month,Week, andDayunit_of_time/__init__.py (3)
277-288: Fix the parameter name.The parameter should be named
idx(notdt) to match the method's purpose and the signatures in all subclass implementations (Year.get_date_from_index,Quarter.get_date_from_index, etc.).Apply this diff:
@abstractmethod - def get_date_from_index(cls, dt): + def get_date_from_index(cls, idx): """ Map an index value for this time unit kind to its corresponding start date. Parameters: - dt (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). + idx (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). Returns: - date_or_none (datetime.date | None): The start date corresponding to `dt`, or `None` when the kind does not provide a mapping. + date_or_none (datetime.date | None): The start date corresponding to `idx`, or `None` when the kind does not provide a mapping. """ return None
566-578: Support datetime input in Week.get_index_for_date.
dt - date.minfails ifdtis adatetime(TypeError: unsupported operand type(s) for -: 'datetime.datetime' and 'datetime.date'). Convert to date first, as documented in the docstring and implemented in other methods liketruncate.Apply this diff:
@classmethod def get_index_for_date(cls, dt): # date.min has weekday() == 0 """ Compute the zero-based week index of a given date relative to date.min (weeks start on Monday). Parameters: dt (datetime.date | datetime.datetime): The date to index; when a datetime is provided, its date component is used. Returns: int: Number of whole weeks between date.min (which is a Monday) and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days // 7
617-628: Support datetime input in Day.get_index_for_date.Same issue as
Week:dt - date.minfails ifdtis adatetime. Convert to date first.Apply this diff:
@classmethod def get_index_for_date(cls, dt): """ Compute the day-based index of a date relative to date.min. Parameters: dt (datetime.date | datetime.datetime): The date to convert into an index. Returns: int: Number of days between `date.min` and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
80-80: Annotate mutable class attributes withClassVar.Static analysis correctly identifies that
_pre_registered,_registered, and_multiplierare class-level mutable attributes that should be annotated withtyping.ClassVarto clarify they're not instance attributes.Apply this diff:
+from typing import ClassVar + class TimeunitKindMeta(IndexableMixin, type): kind_int = None formatter = None - _pre_registered = [] - _registered = None - _multiplier = None + _pre_registered: ClassVar[list] = [] + _registered: ClassVar[dict | None] = None + _multiplier: ClassVar[int | None] = None first_date = date.min last_date = date.max
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)timetest.py(5 hunks)unit_of_time/__init__.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
timetest.py (1)
unit_of_time/__init__.py (23)
get_index_for_date(263-275)get_index_for_date(367-377)get_index_for_date(425-436)get_index_for_date(501-511)get_index_for_date(567-578)get_index_for_date(618-628)get_date_from_index(278-288)get_date_from_index(380-390)get_date_from_index(439-451)get_date_from_index(514-526)get_date_from_index(581-591)get_date_from_index(631-641)last_day(216-226)truncate(300-310)truncate(341-351)truncate(454-464)truncate(594-606)Year(336-405)Quarter(408-478)Month(481-543)Week(546-610)Day(613-670)next(774-775)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(39-49)get_index_for_date(26-36)truncate(13-23)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
80-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
393-393: Unused class method argument: cur
(ARG003)
420-420: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
552-552: Unused class method argument: cur
(ARG003)
644-644: Unused class method argument: cur
(ARG003)
847-847: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: run tests (3.12)
- GitHub Check: run tests (3.9)
- GitHub Check: run tests (3.8)
- GitHub Check: run tests (3.10)
- GitHub Check: run tests (3.11)
🔇 Additional comments (14)
.github/workflows/build.yml (1)
17-26: LGTM!The matrix strategy correctly tests the enhanced time-unit API across Python 3.8–3.12, ensuring broad compatibility.
timetest.py (2)
198-200: LGTM!The test correctly validates the string representation for
Weektype and sliced proxy objects.
241-244: LGTM!These tests correctly validate that:
get_index_for_date(date.min)returns 0 for all time unit kinds- Slicing operations produce equivalent results through different interfaces
unit_of_time/__init__.py (11)
1-7: LGTM!The import of
abstractmethodand theONE_DAYconstant improve code clarity and support the new abstract method definitions.
19-29: LGTM!The docstring clearly documents the date-to-integer encoding format and the optional multiplier parameter.
32-48: LGTM!
IndexableMixin.__getitem__correctly handles both integer indices and slices by delegating torange(len(self))[key], which properly resolves negative indices and slice semantics.
50-75: LGTM!
SlicedProxycorrectly implements lazy slicing by storing the parent and slice, then resolving indices on-demand through the parent's indexing interface.
262-275: LGTM!The abstract method declaration correctly establishes the contract for mapping dates to indices across all time unit kinds.
290-298: LGTM!The
__iter__implementation correctly iterates over all valid time unit indices, making time unit kinds iterable.
336-405: LGTM!The
Yearimplementations correctly compute offset-based indices fromdate.minand handle year boundaries. The unusedcurparameter at line 393 is part of the_inner_shiftinterface contract.
408-478: LGTM!The
Quarterimplementations correctly compute offset-based indices and handle quarter boundaries. The EN DASH at line 420 flagged by static analysis is a minor cosmetic issue that doesn't affect functionality.
481-543: LGTM!The
Monthimplementations correctly compute offset-based indices, handle month arithmetic with year rollover, and map indices back to valid dates.
673-759: LGTM!The
Timeunitclass correctly implements indexable behavior by inheriting fromIndexableMixinand providing_from_indexfor day-level subscripting within each unit. The__iter__method efficiently iterates days using theONE_DAYconstant.
818-847: LGTM!The
_get_rangehelper correctly extracts date ranges from dates, Timeunit instances, or tuples, with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
timetest.py (1)
25-49: Decade indexing pattern already flagged in past reviews.The index calculations for
Decadeare inconsistent with other time units (Year, Month, etc.) which use offset-based indexing fromdate.min. Past review comments have already identified these issues and proposed fixes:
- Line 36: Should use
(dt.year - date.min.year) // 10- Line 49: Should use
date((10 * idx) + date.min.year, 1, 1)
🧹 Nitpick comments (2)
unit_of_time/__init__.py (2)
447-449: Consider using date.min.year for consistency with Year.The formula
date(yy + 1, 3 * qq + 1, 1)works correctly whendate.min.year == 1but hardcodes this assumption.Year.get_date_from_indexexplicitly usesdate.min.year, making the pattern more robust and consistent.Apply this diff to align with
Year's pattern:@classmethod def get_date_from_index(cls, idx): """ Convert a quarter index into the first day of that quarter. Parameters: idx (int): Quarter index where 0 corresponds to year 1, quarter 1; indices increase by one per quarter. Returns: datetime.date: The date for the first day of the quarter (month = 1, 4, 7, or 10) for the computed year. """ yy = idx // 4 qq = idx - 4 * yy - return date(yy + 1, 3 * qq + 1, 1) + return date(yy + date.min.year, 3 * qq + 1, 1)
522-524: Consider using date.min.year for consistency with Year.Same issue as
Quarter: the formuladate(yy + 1, mm, 1)hardcodes the assumption thatdate.min.year == 1. Usingdate.min.yearexplicitly would match the pattern inYear.get_date_from_index.Apply this diff:
@classmethod def get_date_from_index(cls, idx): """ Map a month index to the date of its first day. Parameters: idx (int): Month index where 0 corresponds to 0001-01-01; each increment advances one month. Returns: date: The first day of the month represented by `idx`. """ yy = idx // 12 mm = (idx % 12) + 1 - return date(yy + 1, mm, 1) + return date(yy + date.min.year, mm, 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
timetest.py(6 hunks)unit_of_time/__init__.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
timetest.py (1)
unit_of_time/__init__.py (24)
get_index_for_date(263-274)get_index_for_date(365-375)get_index_for_date(423-434)get_index_for_date(499-509)get_index_for_date(565-576)get_index_for_date(616-626)get_date_from_index(277-286)get_date_from_index(378-388)get_date_from_index(437-449)get_date_from_index(512-524)get_date_from_index(579-589)get_date_from_index(629-639)last_day(216-226)truncate(298-308)truncate(339-349)truncate(452-462)truncate(592-604)Year(334-403)Quarter(406-476)Month(479-541)Week(544-608)Day(611-668)next(772-773)first_date(690-691)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(39-49)get_index_for_date(26-36)truncate(13-23)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
80-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
391-391: Unused class method argument: cur
(ARG003)
418-418: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
550-550: Unused class method argument: cur
(ARG003)
642-642: Unused class method argument: cur
(ARG003)
845-845: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: run tests (3.12)
- GitHub Check: run tests (3.9)
- GitHub Check: run tests (3.10)
- GitHub Check: run tests (3.11)
- GitHub Check: run tests (3.8)
🔇 Additional comments (6)
timetest.py (3)
129-139: LGTM! Index validation logic is thorough.The test correctly validates:
- Index consistency between consecutive units
- Round-trip conversion (date → index → date)
- Subscript access (
kind[idx])- Per-date indexing within each unit (when
dt == tu.first_date)
199-201: Good addition of repr test coverage.
243-246: Test enhancements look good.The slice chaining test
list(kind[10:110:10][5:9:2])validates thatSlicedProxycorrectly supports nested slicing.unit_of_time/__init__.py (3)
32-75: LGTM! IndexableMixin and SlicedProxy are well-designed.The mixin pattern cleanly adds subscript support, and
SlicedProxycorrectly implements lazy slicing with proper__repr__formatting.
77-296: Excellent metaclass enhancements with comprehensive docstrings.The indexing infrastructure is well-documented and the abstract methods clearly define the contract for subclasses.
671-865: Timeunit enhancements are robust and well-documented.The
IndexableMixinintegration, iteration support, and improved error handling in_get_rangestrengthen the public API.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
timetest.py (2)
38-49: Fixget_date_from_indexto match the offset-based pattern.The current implementation has two problems:
Inconsistent with other units: It doesn't add
date.min.yearas a base offset. All other time units inunit_of_time/__init__.pyuse this pattern:
Year.get_date_from_index:date(idx + date.min.year, 1, 1)Month.get_date_from_index:date(yy + date.min.year, mm, 1)Not reversible with
get_index_for_date: After fixingget_index_for_dateto return(dt.year - date.min.year) // 10, the round-trip will break:
- Example:
date(1, 1, 1)→ index0→date(max(0, 1), 1, 1)=date(1, 1, 1)✓ (works by accident)- Example:
date(15, 1, 1)→ index1→date(10, 1, 1)✗ (should bedate(11, 1, 1))Apply this diff to fix the index-to-date conversion:
@classmethod def get_date_from_index(cls, idx): """ Return the start date (January 1) of the decade represented by the given index. Parameters: - idx (int): Decade index; the corresponding year is 10 * idx. + idx (int): Decade index relative to date.min; the corresponding year is (10 * idx) + date.min.year. Returns: - datetime.date: January 1 of the year 10 * idx. + datetime.date: January 1 of the year (10 * idx) + date.min.year. """ - return date(max(10 * idx, 1), 1, 1) + return date((10 * idx) + date.min.year, 1, 1)This ensures:
get_date_from_index(0)returnsdate(1, 1, 1)(valid)- Round-trip works:
get_date_from_index(get_index_for_date(dt)) == cls.truncate(dt)- Pattern matches
Year,Quarter,Month,Week, andDayBased on the pattern established in
unit_of_time/__init__.pyfor all other time unit kinds.
25-36: Fix the index calculation to be consistent with other time units.The current implementation returns the absolute decade number (
dt.year // 10), while all other time units inunit_of_time/__init__.pyreturn offsets fromdate.min. For example:
Year:dt.year - date.min.yearMonth:12 * (dt.year - date.min.year) + dt.month - 1Day:(dt - date.min).daysThis inconsistency causes several issues:
- For
date.min(year 1), this returns0(which happens to be correct by accident)- The pattern breaks if
date.min.yearis not 1- Tests at lines 243 and 129-133 expect
get_index_for_date(date.min) == 0andget_date_from_index(idx)to be reversible, which only works becausedate.min.year == 1Apply this diff to align with the offset-based pattern used by all other time units:
@classmethod def get_index_for_date(cls, dt): """ Return the zero-based decade index for the given date. Parameters: dt (date or datetime): The date for which to compute the decade index. Returns: - int: The decade index equal to the calendar year divided by 10 using integer division (year // 10). + int: The decade index relative to date.min, computed as (year - date.min.year) // 10. """ - return dt.year // 10 + return (dt.year - date.min.year) // 10Based on the pattern established in
unit_of_time/__init__.pyfor all other time unit kinds.unit_of_time/__init__.py (3)
284-293: Inconsistent parameter name in abstract method signature.The parameter is named
dtbut the docstring and all subclass implementations useidx. While Python's@abstractmethoddoesn't enforce parameter names, this inconsistency is confusing for developers and documentation tools.Apply this diff to align the signature with implementations and documentation:
@abstractmethod - def get_date_from_index(cls, dt): + def get_date_from_index(cls, idx): """ Map an index value for this time unit kind to its corresponding start date. Parameters: - dt (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). + idx (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). Returns: - date (datetime.date): The start date corresponding to `dt`. + date (datetime.date): The start date corresponding to `idx`. """
620-631: Handledatetimeinput inDay.get_index_for_date.Same issue as
Week.get_index_for_date- will fail with aTypeErrorif passed adatetime.Apply this diff:
@classmethod def get_index_for_date(cls, dt): """ Compute the day-based index of a date relative to date.min. Parameters: dt (datetime.date | datetime.datetime): The date to convert into an index. Returns: int: Number of days between `date.min` and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days
569-581: Handledatetimeinput inWeek.get_index_for_date.The method will fail with a
TypeErrorif passed adatetimebecause(datetime - date.min).daysis invalid. Thetruncatemethod at lines 607-608 already handles this conversion, butget_index_for_dateshould do the same for consistency.Apply this diff:
@classmethod def get_index_for_date(cls, dt): - # date.min has weekday() == 0 """ Compute the zero-based week index of a given date relative to date.min (weeks start on Monday). Parameters: dt (datetime.date | datetime.datetime): The date to index; when a datetime is provided, its date component is used. Returns: int: Number of whole weeks between date.min (which is a Monday) and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days // 7
🧹 Nitpick comments (1)
timetest.py (1)
23-23: Simplifytruncateafter fixing the indexing formula.The
max(..., 1)guard is a workaround for the incorrect indexing inget_date_from_index. Once you apply the offset-based formula ((10 * idx) + date.min.year), the guard becomes unnecessary because the calculation will never produce year 0 or negative years:
- For decade 0:
(10 * 0) + 1 = 1✓- For decade 1:
(10 * 1) + 1 = 11✓After fixing
get_date_from_index, simplify this to:- return date(max(10 * (dt.year // 10), 1), 1, 1) + return date(10 * (dt.year // 10), 1, 1)Or better yet, reuse the indexing methods for consistency:
- return date(max(10 * (dt.year // 10), 1), 1, 1) + return cls.get_date_from_index(cls.get_index_for_date(dt))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(2 hunks)timetest.py(6 hunks)unit_of_time/__init__.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(39-49)get_index_for_date(26-36)truncate(13-23)
timetest.py (1)
unit_of_time/__init__.py (26)
get_index_for_date(270-281)get_index_for_date(372-382)get_index_for_date(430-441)get_index_for_date(504-514)get_index_for_date(570-581)get_index_for_date(621-631)get_date_from_index(284-293)get_date_from_index(385-395)get_date_from_index(444-456)get_date_from_index(517-529)get_date_from_index(584-594)get_date_from_index(634-644)last_day(222-232)truncate(305-315)truncate(346-356)truncate(459-469)truncate(597-609)Year(341-410)Quarter(413-481)Month(484-546)Week(549-613)Day(616-673)next(778-779)first_date(695-696)last_date(699-700)previous(691-692)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
82-82: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
398-398: Unused class method argument: cur
(ARG003)
425-425: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
555-555: Unused class method argument: cur
(ARG003)
647-647: Unused class method argument: cur
(ARG003)
853-853: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
timetest.py (4)
5-5: LGTM!The
isliceimport is correctly used in the test at line 246 to validate slicing behavior.
129-139: Excellent test coverage for the indexing API.The assertions comprehensively validate:
- Index consistency between adjacent units (line 131-132)
- Round-trip conversion between dates and indices (line 134)
- Subscript access via
kind[idx](line 135)- Per-unit index consistency for all dates within a unit (lines 136-139)
The conditional check at line 136 (
if dt == tu.first_date) efficiently prevents O(N^2) runtime while still ensuring thorough coverage for each time unit.
199-201: LGTM!The test correctly validates the
__repr__implementation for both theWeekkind and sliced proxies.
243-246: LGTM!The assertions correctly validate:
- Line 243: All kinds return index 0 for
date.min(this will catch the Decade indexing issue)- Line 245: Slice chaining produces correct results
- Line 246:
isliceand subscript slicing are equivalentunit_of_time/__init__.py (5)
34-49: LGTM!The
IndexableMixincorrectly implements subscript access with proper handling of both integer indices and slices. The use ofrange(len(self))[key]elegantly handles negative indices and slice semantics.
52-77: LGTM!The
SlicedProxycorrectly implements a lazy slice view with:
- Proper delegation to the parent via
_from_index- Efficient iteration without materializing the full sequence
- Clear
__repr__showing the slice notation
371-395: LGTM!The
get_index_for_dateandget_date_from_indeximplementations forYear,Quarter, andMonthcorrectly follow the offset-based indexing pattern relative todate.min:
- Year: Simple year offset
- Quarter:
4 * year_offset + quarter_in_year- Month:
12 * year_offset + month_in_yearAll implementations ensure reversibility:
get_date_from_index(get_index_for_date(dt)) == truncate(dt).Also applies to: 430-456, 504-529
677-688: LGTM!The
Timeunit.__init__correctly:
- Resolves kind from the registry when an integer is provided
- Truncates the input date to the unit boundary for consistency
- Has comprehensive documentation
740-750: LGTM!The
_from_indexmethod correctly implements day-based indexing within a time unit, allowing subscript access liketu[0]for the first day,tu[-1]for the last day, etc..github/workflows/build.yml (1)
20-20: LGTM!The CI enhancements are excellent:
- Expanded Python version matrix (3.8-3.12 for tests, 3.9-3.13 for builds) ensures broad compatibility
- Addition of
pytest-random-orderhelps catch test interdependencies and improves test quality- Maintains 100% code coverage requirement
Also applies to: 30-31, 36-43
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
timetest.py (1)
23-49: Align Decade indexing with other units (relative to date.min) and remove max() clampsMake decade index/date computations consistent with Year/Quarter/Month/Week/Day. Also handle datetime input in truncate.
- return date(max(10 * (dt.year // 10), 1), 1, 1) + if isinstance(dt, datetime): + dt = dt.date() + base = date.min.year + # First day of the decade containing dt, relative to date.min + return date(10 * ((dt.year - base) // 10) + base, 1, 1) @@ - def get_index_for_date(cls, dt): + def get_index_for_date(cls, dt): @@ - int: The decade index equal to the calendar year divided by 10 using integer division (year // 10). + int: The decade index relative to date.min, computed as (dt.year - date.min.year) // 10. @@ - return dt.year // 10 + return (dt.year - date.min.year) // 10 @@ - def get_date_from_index(cls, idx): + def get_date_from_index(cls, idx): @@ - idx (int): Decade index; the corresponding year is 10 * idx. + idx (int): Decade index relative to date.min; the corresponding year is (10 * idx) + date.min.year. @@ - datetime.date: January 1 of the year 10 * idx. + datetime.date: January 1 of the year (10 * idx) + date.min.year. @@ - return date(max(10 * idx, 1), 1, 1) + return date((10 * idx) + date.min.year, 1, 1)unit_of_time/__init__.py (2)
569-582: Support datetime input in Week.get_index_for_dateSubtracting a datetime from
date.minraises TypeError. Convert to date first.def get_index_for_date(cls, dt): - # date.min has weekday() == 0 - """ + # date.min has weekday() == 0 + """ @@ - return (dt - date.min).days // 7 + if isinstance(dt, datetime): + dt = dt.date() + return (dt - date.min).days // 7
620-632: Support datetime input in Day.get_index_for_dateMirror Week: accept datetime by converting to date.
def get_index_for_date(cls, dt): @@ - return (dt - date.min).days + if isinstance(dt, datetime): + dt = dt.date() + return (dt - date.min).days
🧹 Nitpick comments (6)
timetest.py (1)
98-112: Avoid OOM: replace massive boolean list with a sparse map
d = [False] * 202101019allocates hundreds of millions of entries. Use a defaultdict keyed byint(tu).- d = [False] * 202101019 + d = defaultdict(bool) @@ - self.assertEqual(d[tu], tu in cur_set) - self.assertEqual(d[tu], int(tu) in cur_set) - d[tu] = True + self.assertEqual(d[int(tu)], tu in cur_set) + self.assertEqual(d[int(tu)], int(tu) in cur_set) + d[int(tu)] = TrueAdd import (outside this hunk):
from collections import defaultdictunit_of_time/__init__.py (5)
8-9: Remove duplicate ONE_DAY definitionONE_DAY is defined twice. Keep a single definition.
-ONE_DAY = timedelta(days=1) -
163-174: Duplicate int on metaclass; remove the first oneTwo int methods are defined back-to-back; the first is redundant and shadowed.
- def __int__(self): - """ - Provide the integer identifier for this time unit kind. - """ - return self.kind_int - - def __int__(cls): + def __int__(cls): return cls.kind_int
283-294: Rename abstract param to idx for consistency with subclassesUnify parameter naming to avoid confusion and align with Year/Quarter/Month/Week/Day implementations.
- def get_date_from_index(cls, dt): + def get_date_from_index(cls, idx): @@ - dt (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). + idx (int): Integer index representing the offset of the unit (e.g., number of days/weeks/months/years since date.min). @@ - date (datetime.date): The start date corresponding to `dt`. + date (datetime.date): The start date corresponding to `idx`.
82-87: Annotate mutable class attributes as ClassVar (style/ruff RUF012)_pre_registered, _registered, and _multiplier are class-level caches; annotate as ClassVar to avoid instance shadowing warnings.
Add:
from typing import ClassVar class TimeunitKindMeta(IndexableMixin, type): _pre_registered: ClassVar[list] = [] _registered: ClassVar[dict] | None = None _multiplier: ClassVar[int] | None = None first_date: ClassVar[date] = date.min last_date: ClassVar[date] = date.max
853-854: Minor: simplify exception message (TRY003 hint)Shorten or reuse a standard message to avoid long f-strings in exceptions.
- raise TypeError(f"Item {item!r} has no date range.") + raise TypeError("Item has no date range.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(2 hunks)timetest.py(6 hunks)unit_of_time/__init__.py(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🧬 Code graph analysis (2)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(39-49)get_index_for_date(26-36)truncate(13-23)
timetest.py (1)
unit_of_time/__init__.py (26)
get_index_for_date(270-281)get_index_for_date(372-382)get_index_for_date(430-441)get_index_for_date(504-514)get_index_for_date(570-581)get_index_for_date(621-631)get_date_from_index(284-293)get_date_from_index(385-395)get_date_from_index(444-456)get_date_from_index(517-529)get_date_from_index(584-594)get_date_from_index(634-644)last_day(222-232)truncate(305-315)truncate(346-356)truncate(459-469)truncate(597-609)Year(341-410)Quarter(413-481)Month(484-546)Week(549-613)Day(616-673)next(778-779)first_date(695-696)last_date(699-700)previous(691-692)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
82-82: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
398-398: Unused class method argument: cur
(ARG003)
425-425: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
555-555: Unused class method argument: cur
(ARG003)
647-647: Unused class method argument: cur
(ARG003)
853-853: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: run tests (3.9)
- GitHub Check: run tests (3.12)
- GitHub Check: run tests (3.10)
- GitHub Check: run tests (3.8)
- GitHub Check: run tests (3.11)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
README.md (2)
110-110: Fix inline code formatting.The inline code span for
Monthis not closed before the comma. It should read`Month`, etc.not`Month, etc.. This causes the comma to be included inside the code span.Additionally, the static analysis tool flags spaces inside code span elements. The ellipsis notation in
.get_index_for_date(…)and.get_date_from_index(…)appears correct, but please verify there are no extra spaces before or after the parentheses within the backticks.Apply this diff to fix the
Monthcode span:-The `Day`, `Week`, `Month, etc. classes have `.get_index_for_date(…)` and `.get_date_from_index(…)` methods, which allow to determine how many days, weeks, months, quarters and years are between `date.min` and the date given, and convert this back to a date. For example: +The `Day`, `Week`, `Month`, etc. classes have `.get_index_for_date(...)` and `.get_date_from_index(...)` methods, which allow to determine how many days, weeks, months, quarters and years are between `date.min` and the date given, and convert this back to a date. For example:
119-119: Correct the class name typo.Line 119 contains "TimUnit" which should be "Timeunit" to match the actual class name.
Apply this diff:
-We can also use the index to get a `TimUnit` with: +We can also use the index to get a `Timeunit` with:unit_of_time/__init__.py (2)
563-575: Add datetime support inget_index_for_date.The subtraction
(dt - date.min)will raise aTypeErrorifdtis adatetimeinstance, since Python doesn't support mixeddatetime/datearithmetic. The docstring promises support for bothdateanddatetimeinputs.Apply this diff to handle
datetimeinputs:@classmethod def get_index_for_date(cls, dt): - # date.min has weekday() == 0 """ Compute the zero-based week index of a given date relative to date.min (weeks start on Monday). Parameters: dt (datetime.date | datetime.datetime): The date to index; when a datetime is provided, its date component is used. Returns: int: Number of whole weeks between date.min (which is a Monday) and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days // 7
614-625: Add datetime support inget_index_for_date.The subtraction
(dt - date.min)will raise aTypeErrorifdtis adatetimeinstance. The docstring specifies support for bothdateanddatetimeinputs.Apply this diff to handle
datetimeinputs:@classmethod def get_index_for_date(cls, dt): """ Compute the day-based index of a date relative to date.min. Parameters: dt (datetime.date | datetime.datetime): The date to convert into an index. Returns: int: Number of days between `date.min` and `dt`. """ + if isinstance(dt, datetime): + dt = dt.date() return (dt - date.min).days
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)
419-419: Consider using hyphen-minus instead of en-dash.The docstring contains an en-dash (–) where a hyphen-minus (-) is more conventional for numeric ranges in code documentation.
Apply this diff if desired:
- quarter_str (str): A string in the form `YYYYQn` where `n` is the quarter number (1–4). + quarter_str (str): A string in the form `YYYYQn` where `n` is the quarter number (1-4).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)unit_of_time/__init__.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
get_date_from_index(39-49)get_index_for_date(26-36)truncate(13-23)
🪛 markdownlint-cli2 (0.18.1)
README.md
110-110: Spaces inside code span elements
(MD038, no-space-in-code)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py
79-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
392-392: Unused class method argument: cur
(ARG003)
419-419: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
549-549: Unused class method argument: cur
(ARG003)
641-641: Unused class method argument: cur
(ARG003)
847-847: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: run tests (3.12)
- GitHub Check: run tests (3.9)
- GitHub Check: run tests (3.11)
- GitHub Check: run tests (3.10)
- GitHub Check: run tests (3.8)
🔇 Additional comments (5)
unit_of_time/__init__.py (5)
31-46: LGTM: Clean indexing mixin.The
IndexableMixinimplementation correctly delegates to Python'srangefor index normalization and slice handling, then routes integer keys to_from_indexand slices toSlicedProxy. The docstring clearly documents the contract.
49-73: LGTM: Lazy slice view implementation.The
SlicedProxycorrectly implements a lazy slice view that supports further indexing and slicing. The__repr__method provides clear slice notation, and the implementation avoids materializing elements until iteration.
263-287: LGTM: Abstract methods properly defined.The abstract methods
get_index_for_dateandget_date_from_indexare correctly decorated and documented. The docstrings specify that bothdateanddatetimeinputs should be supported.
365-389: LGTM: Year indexing implementation.The
get_index_for_dateandget_date_from_indeximplementations forYearare straightforward and correct. The index is simply the offset fromdate.min.year, and the conversion back is equally direct.
670-757: LGTM: Timeunit enhancements for indexing.The
Timeunitclass correctly inherits fromIndexableMixinand implements the required_from_indexmethod to support subscripting. The__init__properly truncates the input date and stores the kind. The updated__len__and__iter__methods provide clear semantics for iterating over days within a time unit.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores