Skip to content

Conversation

@KommuSoft
Copy link
Member

@KommuSoft KommuSoft commented Aug 30, 2025

add pyright

Summary by CodeRabbit

  • New Features

    • None
  • Bug Fixes

    • More predictable date-range handling in time-unit operations.
    • Safer comparisons and out-of-range behavior for integer-based lookups.
  • Refactor

    • Widespread explicit typing across time-unit APIs and stricter date-only signatures.
  • Tests

    • CI now runs static type checks and import-sorting checks as separate checks.
  • Chores

    • CI workflow updated: checkout action upgraded; publish now depends on type checks; import-sorting check targets the time-unit package.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 30, 2025

Walkthrough

Adds separate Pyright and isort CI jobs and bumps Actions versions; applies pervasive static typing and signature annotations across unit_of_time/init.py and adjusts import ordering in timetest.py.

Changes

Cohort / File(s) Change Summary
CI: add pyright & isort, bump actions
.github/workflows/build.yml
Upgrades actions/checkout to @v4 and actions/setup-python to @v5; adds pyright job (installs pyright>=1.1,<2 and runs pyright) and isort job (installs isort>=6.0,<6.1 and runs isort -c unit_of_time); updates publish job needs to include pyright.
Typing and API annotations
unit_of_time/__init__.py
Adds extensive type annotations and typed imports (e.g., date_from_int: (int)->date, date_to_int: (date)->int), typed class attributes and registries (kind_int, formatter, _pre_registered, _registered, _multiplier), and annotated method signatures across TimeunitKindMeta, concrete Timeunit kinds, and Timeunit. Defaults tightened (e.g., kind_int=-1, formatter=""); safer out-of-range handling added. Behavioral semantics preserved.
Import formatting (test)
timetest.py
Reformats/reorders the import list (symbols unchanged; import order adjusted).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant GH as GitHub Actions
  participant Runner as Runner (ubuntu-latest)
  participant SetupPy as actions/setup-python@v5
  participant Pyright as pyright
  participant Isort as isort

  Dev->>GH: push / open PR
  GH->>Runner: start "pyright" job
  Runner->>Runner: actions/checkout@v4
  Runner->>SetupPy: setup python (3.x, cache: pip)
  Runner->>Runner: pip install pyright>=1.1,<2
  Runner->>Pyright: run pyright → diagnostics
  GH->>Runner: start "isort" job
  Runner->>Runner: actions/checkout@v4
  Runner->>SetupPy: setup python (3.x, cache: pip)
  Runner->>Runner: pip install isort>=6.0,<6.1
  Runner->>Isort: run isort -c unit_of_time → diagnostics
  Note over GH,Runner: publish job now requires successful pyright (in addition to black/build/test)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Shifting over time units #10 — Overlaps edits to time-unit shifting APIs and related signatures in unit_of_time/__init__.py.
  • pre-release #7 — Related CI/workflow changes affecting jobs and publish dependencies.
  • pylint matrix #11 — Also modifies unit_of_time/__init__.py and adds CI/linting jobs; likely overlapping edits.

Poem

I hop through typed and tidy code,
Pyright checks each careful node.
Isort lines up tails so neat,
CI hums a steady beat.
Carrot compile — victory! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.21% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “add pyright” accurately notes the new pyright CI job but omits other significant changes like isort integration and the extensive type annotation updates, so it only partially reflects the pull request’s scope.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch issue/typing

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (9)
.github/workflows/build.yml (3)

15-27: Pyright CI job: solid addition; consider pinning and pip caching.

  • Pin pyright to a known-good range to avoid surprise CI breaks.
  • Enable pip cache for faster runs.

Apply:

     - name: Set up Python
       uses: actions/setup-python@v5
       with:
-        python-version: '3.x'
+        python-version: '3.x'
+        cache: 'pip'
     - name: Run pyright
       run: |
-        pip install pyright
-        pyright unit_of_time/*.py
+        pip install "pyright>=1.1,<2"
+        pyright

61-63: Gate publishing on type-checking too.

Include pyright in publish prerequisites so releases block on type health.

-    needs: [black, build, test]
+    needs: [black, build, test, pyright]

11-12: Unify checkout action to v4 for consistency.

Minor consistency nit: the black job still uses v2.

-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v4
unit_of_time/__init__.py (6)

6-12: Type the parameter for clarity.

Annotate div to satisfy pyright and readers.

-def date_from_int(val: int, div=1) -> date:
+def date_from_int(val: int, div: int = 1) -> date:

146-153: Silence unused-parameter warnings in the abstract _inner_shift.

Prefix unused params to document intent.

-    def _inner_shift(cls, cur, dt, amount) -> Union[date, None]:
+    def _inner_shift(cls, _cur, _dt, _amount) -> Union[date, None]:
         return None

231-240: Annotation doesn’t match runtime for Week.truncate.

You accept datetime; reflect that in the type.

-    def truncate(cls, dt: date) -> date:
+    def truncate(cls, dt: date | datetime) -> date:
         if isinstance(dt, datetime):
             dt = dt.date()
         return dt - timedelta(days=dt.weekday())

251-256: Unused parameter in Day._inner_shift.

Minor clean-up to avoid lints.

-    def _inner_shift(cls, cur: "Timeunit", dt: date, amount: int) -> date:
+    def _inner_shift(cls, _cur: "Timeunit", dt: date, amount: int) -> date:
         return dt + timedelta(days=amount)

195-200: Tiny clean-up: remove dead local and use computed y.

No behavior change.

-        q_new = dt.year * 4 + amount + (dt.month - 1) // 3
-        y = q_new // 4
-        q = q_new % 4
-        return date(q_new // 4, 3 * q + 1, 1)
+        q_new = dt.year * 4 + amount + (dt.month - 1) // 3
+        y, q = divmod(q_new, 4)
+        return date(y, 3 * q + 1, 1)

377-407: Make _get_range exception flow clearer; align with lints.

Avoid raising inside the try and collapse messages.

-        # try to make a range
-        try:
-            dt0, dt1 = item
-            if isinstance(dt0, date) and isinstance(dt1, date):
-                return dt0, dt1
-            raise TypeError(f"Item {item!r} is not a date range.") from None
-        except TypeError:
-            raise TypeError(f"Item {item!r} has no date range.") from None
+        # try to make a range
+        try:
+            dt0, dt1 = item
+        except Exception:
+            raise TypeError(f"Item {item!r} has no date range.") from None
+        if isinstance(dt0, date) and isinstance(dt1, date):
+            return dt0, dt1
+        raise TypeError(f"Item {item!r} is not a date range.") from None
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between cda9b9f and 73d386d.

📒 Files selected for processing (2)
  • .github/workflows/build.yml (1 hunks)
  • unit_of_time/__init__.py (14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
  • last_day (25-36)
  • truncate (12-22)
  • test_to_int (56-155)
🪛 Ruff (0.12.2)
unit_of_time/__init__.py

22-22: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


146-146: Unused method argument: cur

(ARG002)


146-146: Unused method argument: dt

(ARG002)


146-146: Unused method argument: amount

(ARG002)


251-251: Unused class method argument: cur

(ARG003)


404-404: Abstract raise to an inner function

(TRY301)


404-404: 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 (1)
unit_of_time/__init__.py (1)

106-129: Potential mutual-recursion trap between last_day and _next.

Base last_day calls _next and base _next calls last_day. Ensure every concrete TimeunitKind overrides at least one to avoid infinite recursion.

Would you like me to add a defensive NotImplementedError in one base method to force overrides?

Comment on lines 1 to 3
import math
from datetime import date, datetime, timedelta
from typing import Dict, Union
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Python 3.8 compatibility: annotations will break without future import.

You’re using PEP 585 generics (list[...] / tuple[...]) while supporting 3.8. Add the future import (or switch to typing.List/Tuple). This is a real runtime error on 3.8.

+from __future__ import annotations
 import math
 from datetime import date, datetime, timedelta
-from typing import Dict, Union
+from typing import Dict, Union, ClassVar, Mapping, Type
📝 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.

Suggested change
import math
from datetime import date, datetime, timedelta
from typing import Dict, Union
from __future__ import annotations
import math
from datetime import date, datetime, timedelta
from typing import Dict, Union, ClassVar, Mapping, Type
🤖 Prompt for AI Agents
In unit_of_time/__init__.py lines 1-3, the module may use PEP 585-style
annotations which break at runtime on Python 3.8; add the future import by
inserting "from __future__ import annotations" as the very first line of the
file (before any other imports) to enable postponed evaluation of annotations,
or alternatively replace PEP 585 types with typing.List, typing.Tuple, etc.,
throughout the file to maintain 3.8 compatibility.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 (2)
unit_of_time/__init__.py (2)

33-43: Expose a read-only mapping of kind_int → kind class.

Return Mapping[int, type["TimeunitKind"]] and keep the cached dict in the ClassVar.

-    def unit_register(self) -> Dict[int, "TimeunitKindMeta"]:
+    def unit_register(self) -> Mapping[int, type["TimeunitKind"]]:
         result = TimeunitKindMeta._registered
         if result is None:
-            result = {
-                k.kind_int: k
-                for k in TimeunitKindMeta._pre_registered
-                if k.kind_int is not None
-            }
+            result = {k.kind_int: k for k in TimeunitKindMeta._pre_registered if k.kind_int is not None}
             TimeunitKindMeta._registered = result
         return result

187-189: Fix quarter label off-by-one.

dt.month // 3 yields Q0 for Jan/Feb. Use 1–4.

     def to_str(cls, dt):
-        return f"{dt.year}Q{dt.month//3}"
+        return f"{dt.year}Q{(dt.month - 1)//3 + 1}"
♻️ Duplicate comments (2)
unit_of_time/__init__.py (2)

20-24: Class variables should be ClassVar and store kind classes, not the metaclass.

Fixes RUF012 and aligns types with actual contents.

-    kind_int: int = -1
-    formatter: str = ""
-    _pre_registered: list["TimeunitKindMeta"] = []
-    _registered: Union[None, Dict[int, "TimeunitKindMeta"]] = None
-    _multiplier: int = -1
+    kind_int: int = -1
+    formatter: str = ""
+    _pre_registered: ClassVar[list[type["TimeunitKind"]]] = []
+    _registered: ClassVar[Dict[int, type["TimeunitKind"]] | None] = None
+    _multiplier: ClassVar[int] = -1

1-3: Add future import to avoid runtime crashes on Python 3.8.

You use PEP 585 types (e.g., tuple[...]) elsewhere. Without from __future__ import annotations, this will fail at runtime on 3.8. Also extend typing imports for upcoming fixes.

+from __future__ import annotations
 import math
 from datetime import date, datetime, timedelta
-from typing import Dict, Union
+from typing import ClassVar, Dict, Mapping, Type, Union
🧹 Nitpick comments (9)
.github/workflows/build.yml (1)

11-12: Upgrade checkout action for Black to v4 for consistency.

Keep actions aligned across jobs to avoid deprecation drift.

-      - uses: actions/checkout@v2
+      - uses: actions/checkout@v4
unit_of_time/__init__.py (8)

6-12: Type the div parameter and guard invalid divisors.

Annotate div and optionally protect against zero/negative divisors.

-def date_from_int(val: int, div=1) -> date:
+def date_from_int(val: int, div: int = 1) -> date:
+    if div <= 0:
+        raise ValueError("div must be a positive integer")

46-54: Simplify multiplier calculation using max kind.

Current per-kind while-loop works but is harder to reason about.

     def multiplier(cls) -> int:
         result = TimeunitKindMeta._multiplier
         if result == -1:
-            result = 1
-            for k in TimeunitKindMeta._pre_registered:
-                while k.kind_int >= result:
-                    result *= 10
+            max_kind = max((k.kind_int for k in TimeunitKindMeta._pre_registered if k.kind_int is not None), default=0)
+            result = 1
+            while result <= max_kind:
+                result *= 10
             TimeunitKindMeta._multiplier = result
         return result

96-99: Handle invalid integers in from_int with a clearer error.

Indexing will raise KeyError; surface a TypeError with context.

     def from_int(cls, val: int) -> "Timeunit":
         mul = cls.multiplier
-        return TimeunitKind.unit_register[val % mul](date_from_int(val, mul))
+        kind = TimeunitKind.unit_register.get(val % mul)
+        if kind is None:
+            raise TypeError(f"Unknown timeunit kind for value {val!r}")
+        return kind(date_from_int(val, mul))

140-145: Annotate dt for to_str/truncate for consistency.

Minor clarity; helps Pyright.

-    def to_str(cls, dt) -> str:
+    def to_str(cls, dt: date) -> str:
         return dt.strftime(cls.formatter)
@@
-    def truncate(cls, dt: date) -> date:
+    def truncate(cls, dt: date) -> date:
         return datetime.strptime(cls.to_str(dt), cls.formatter).date()

146-153: Type parameters of _inner_shift to match _shift usage.

Also silence unused args in default impl.

-    def _inner_shift(cls, cur, dt, amount) -> Union[date, None]:
-        return None
+    def _inner_shift(cls, cur: "Timeunit", dt: date, amount: int) -> Union[date, None]:
+        _ = (cur, dt, amount)
+        return None

195-200: Remove unused local and use computed year var.

Tiny cleanup; avoids confusion.

     def _inner_shift(cls, cur: "Timeunit", dt: date, amount: int) -> date:
         q_new = dt.year * 4 + amount + (dt.month - 1) // 3
-        y = q_new // 4
-        q = q_new % 4
-        return date(q_new // 4, 3 * q + 1, 1)
+        y = q_new // 4
+        q = q_new % 4
+        return date(y, 3 * q + 1, 1)

232-240: Annotate Week.truncate to accept datetime explicitly.

The body handles datetime; reflect that in the type.

-    def truncate(cls, dt: date) -> date:
+    def truncate(cls, dt: Union[date, datetime]) -> date:
         if isinstance(dt, datetime):
             dt = dt.date()
         return dt - timedelta(days=dt.weekday())

377-405: PEP 585 in 3.8: prefer typing.Tuple or ensure future import.

If you adopt the future import (see earlier comment), this is fine. Otherwise, switch to Tuple[date, date].

-    def _get_range(cls, item) -> tuple[date, date]:
+    def _get_range(cls, item) -> Tuple[date, date]:

If you prefer to keep tuple[...], please confirm from __future__ import annotations is present at file top (Line 1) so 3.8 doesn’t evaluate it at runtime.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 73d386d and a0736d7.

📒 Files selected for processing (2)
  • .github/workflows/build.yml (2 hunks)
  • unit_of_time/__init__.py (15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (3)
  • last_day (25-36)
  • truncate (12-22)
  • test_to_int (56-155)
🪛 Ruff (0.12.2)
unit_of_time/__init__.py

22-22: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


146-146: Unused method argument: cur

(ARG002)


146-146: Unused method argument: dt

(ARG002)


146-146: Unused method argument: amount

(ARG002)


179-179: Unused class method argument: cur

(ARG003)


195-195: Unused class method argument: cur

(ARG003)


214-214: Unused class method argument: cur

(ARG003)


232-232: Unused class method argument: cur

(ARG003)


251-251: Unused class method argument: cur

(ARG003)


404-404: Abstract raise to an inner function

(TRY301)


404-404: 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 (1)
.github/workflows/build.yml (1)

63-63: Good: publish gated on pyright.

Including pyright in the publish gate strengthens release quality.

Comment on lines 15 to 28
pyright:
name: pyright
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.x'
cache: 'pip'
- name: Run pyright
run: |
pip install "pyright>=1.1,<2"
pyright
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make Pyright check against Python 3.8 and cache deps.

  • The library supports 3.8–3.12; run Pyright with pythonVersion=3.8 to catch 3.8-incompatible annotations (e.g., PEP 585 generics).
  • Add typing-extensions for 3.8-only typing gaps.
  • Improve pip caching with cache-dependency-path.

Apply:

   pyright:
     name: pyright
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v4
     - name: Set up Python
       uses: actions/setup-python@v5
       with:
         python-version: '3.x'
-        cache: 'pip'
+        cache: 'pip'
+        cache-dependency-path: |
+          requirements*.txt
+          setup.cfg
+          setup.py
+          pyproject.toml
     - name: Run pyright
       run: |
-        pip install "pyright>=1.1,<2"
-        pyright
+        python -m pip install -U pip
+        pip install "pyright>=1.1,<2" typing-extensions
+        pyright --pythonversion 3.8
📝 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.

Suggested change
pyright:
name: pyright
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.x'
cache: 'pip'
- name: Run pyright
run: |
pip install "pyright>=1.1,<2"
pyright
pyright:
name: pyright
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.x'
cache: 'pip'
cache-dependency-path: |
requirements*.txt
setup.cfg
setup.py
pyproject.toml
- name: Run pyright
run: |
python -m pip install -U pip
pip install "pyright>=1.1,<2" typing-extensions
pyright --pythonversion 3.8
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 15 to 28: update the pyright job to
install typing-extensions and run pyright targeting Python 3.8, and improve pip
caching by adding cache-dependency-path; specifically, in the Set up Python step
add cache-dependency-path: requirements.txt (or your lock file if you use
pyproject/poetry), and in the Run pyright step install typing-extensions
alongside pyright (pip install "pyright>=1.1,<2" "typing-extensions") and invoke
pyright with --pythonversion 3.8 (pyright --pythonversion 3.8).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
.github/workflows/build.yml (1)

19-28: Run Pyright against Python 3.8, drop unnecessary setuptools install, and improve pip cache.

Targets oldest supported version and speeds CI while catching 3.8-incompatible typing (PEP 585/604). Apply:

     - name: Set up Python
       uses: actions/setup-python@v5
       with:
         python-version: '3.x'
-        cache: 'pip'
+        cache: 'pip'
+        cache-dependency-path: |
+          requirements*.txt
+          setup.cfg
+          setup.py
+          pyproject.toml
     - name: Run pyright
       run: |
-        pip install "pyright>=1.1,<2" "setuptools>=38.6.0,<69.0"
-        pyright
+        python -m pip install -U pip
+        pip install "pyright>=1.1,<2" typing-extensions
+        pyright --pythonversion 3.8

To ensure 3.8 runtime compatibility with PEP 585/604 syntax used in type hints, verify that from __future__ import annotations is present where needed (e.g., unit_of_time/init.py):

#!/bin/bash
# Verify future annotations import for Python 3.8 compatibility
rg -nP "^from __future__ import\s+annotations" -C2 -- unit_of_time/__init__.py
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)

39-42: Align isort with Black to avoid formatter conflicts.

Use Black profile so isort’s output matches Black:

-        pip install "isort>=6.0,<6.1"
-        isort -c .
+        pip install "isort>=6.0,<6.1"
+        isort -c --profile black .
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a0736d7 and 9b5684c.

📒 Files selected for processing (2)
  • .github/workflows/build.yml (2 hunks)
  • timetest.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
timetest.py (1)
unit_of_time/__init__.py (7)
  • Day (246-256)
  • Month (209-224)
  • Quarter (183-206)
  • Timeunit (259-428)
  • TimeunitKind (165-167)
  • Week (227-243)
  • Year (170-180)
🪛 GitHub Actions: CI/CD
timetest.py

[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black --write' to fix code style issues.

🔇 Additional comments (2)
.github/workflows/build.yml (2)

11-11: LGTM: checkout upgraded to v4.

Good move to keep actions current.


77-77: LGTM: gate publish on pyright too.

Publishing only after type checks pass is a solid safeguard.

timetest.py Outdated
Comment on lines 4 to 5
from unit_of_time import (Day, Month, Quarter, Timeunit, TimeunitKind, Week,
Year)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Black failure: format import with trailing comma and one-name-per-line.

Black reports one file to reformat; this multi-line import is the likely culprit. Apply:

-from unit_of_time import (Day, Month, Quarter, Timeunit, TimeunitKind, Week,
-                          Year)
+from unit_of_time import (
+    Day,
+    Month,
+    Quarter,
+    Timeunit,
+    TimeunitKind,
+    Week,
+    Year,
+)
📝 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.

Suggested change
from unit_of_time import (Day, Month, Quarter, Timeunit, TimeunitKind, Week,
Year)
from unit_of_time import (
Day,
Month,
Quarter,
Timeunit,
TimeunitKind,
Week,
Year,
)
🤖 Prompt for AI Agents
In timetest.py around lines 4 to 5, the multi-name multi-line import causes
Black to fail; reformat the import so each name is on its own line inside the
parentheses, include a trailing comma after the last name, and ensure
indentation matches project style (one-name-per-line with closing paren on its
own line) so Black will accept the file.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
unit_of_time/__init__.py (2)

1-2: This issue was previously flagged but remains unresolved.

The code uses PEP 585 generics (list[...], dict[...], tuple[...]) which break at runtime on Python 3.8. The past review comment suggested adding from __future__ import annotations as the first import, but this hasn't been applied yet.

Apply this diff to fix Python 3.8 compatibility:

+from __future__ import annotations
 from datetime import date, datetime, timedelta
 from typing import Dict, Union

21-21: Mutable class attribute needs ClassVar annotation.

This was flagged in the previous review but hasn't been fixed. Class-level mutable attributes must be annotated with ClassVar to satisfy type checkers.

Apply this diff:

+from typing import Dict, Union, ClassVar
-    _pre_registered: list["TimeunitKindMeta"] = []
+    _pre_registered: ClassVar[list["TimeunitKindMeta"]] = []

Similarly, update lines 22-23:

-    _registered: None | Dict[int, "TimeunitKindMeta"] = None
-    _multiplier: int = -1
+    _registered: ClassVar[None | Dict[int, "TimeunitKindMeta"]] = None
+    _multiplier: ClassVar[int] = -1
🧹 Nitpick comments (1)
unit_of_time/__init__.py (1)

47-52: Consider optimizing multiplier computation.

The current nested-loop implementation is correct but inefficient. It processes each registered kind individually when you only need to find the maximum kind_int once.

Apply this diff for a cleaner, more efficient approach:

     if result == -1:
-        result = 1
-        for k in TimeunitKindMeta._pre_registered:
-            while k.kind_int >= result:
-                result *= 10
+        max_kind = max((k.kind_int for k in TimeunitKindMeta._pre_registered if k.kind_int is not None), default=0)
+        result = 1
+        while result <= max_kind:
+            result *= 10
         TimeunitKindMeta._multiplier = result
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e525a25 and 9e14c97.

📒 Files selected for processing (1)
  • unit_of_time/__init__.py (15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
unit_of_time/__init__.py (1)
timetest.py (2)
  • last_day (25-36)
  • truncate (12-22)
🪛 Ruff (0.13.3)
unit_of_time/__init__.py

21-21: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


145-145: Unused method argument: cur

(ARG002)


145-145: Unused method argument: dt

(ARG002)


145-145: Unused method argument: amount

(ARG002)


178-178: Unused class method argument: cur

(ARG003)


194-194: Unused class method argument: cur

(ARG003)


213-213: Unused class method argument: cur

(ARG003)


231-231: Unused class method argument: cur

(ARG003)


250-250: Unused class method argument: cur

(ARG003)


403-403: Abstract raise to an inner function

(TRY301)


403-403: 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 (2)
unit_of_time/__init__.py (2)

376-405: LGTM! Type-safe date range extraction.

The implementation correctly handles all input types (date, Timeunit, tuple) with appropriate type checking and error messages. The return type annotation tuple[date, date] properly reflects all return paths.


5-426: Excellent type annotation coverage.

The comprehensive addition of type hints throughout the module significantly improves type safety and IDE support. The parameter and return type annotations are accurate and will help catch type errors early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants