-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Three small, related clarity issues — bundled because none alone justifies a separate issue.
1. EventBase is dead code (models.py:215–222)
EventBase is defined with the comment "useful for filtering / indexing" but is never imported or used in any production code (src/). It partially duplicates SessionEvent's envelope fields. It adds maintenance burden with no payoff.
Fix: Remove EventBase from models.py and its single test reference in test_models.py.
2. Confusing duration-formatter names (report.py:61 and report.py:91)
Two functions have nearly identical names but do fundamentally different things:
| Name | Signature | What it does |
|---|---|---|
format_duration (public) |
(ms: int) → str |
Formats a millisecond count as "6m 29s" |
_format_duration (private) |
(start: datetime) → str |
Computes elapsed time from start to now, formats as "Xh Ym" |
The private one should be renamed to _format_elapsed_since (or similar) to make the distinction obvious at call sites.
Fix: Rename _format_duration → _format_elapsed_since throughout report.py.
3. Non-idiomatic default_factory for toolRequests (models.py:107)
# current — confusing: list[dict[str, object]] is a type, calling it returns []
toolRequests: list[dict[str, object]] = Field(
default_factory=lambda: list[dict[str, object]]()
)
# idiomatic
toolRequests: list[dict[str, object]] = Field(default_factory=list)The current form works because list[X]() delegates to list(), but it reads as if the type annotation is being instantiated, confusing contributors.
Fix: Replace with default_factory=list.
Testing Requirements
- After removing
EventBase: ensure all tests still pass; remove theEventBasetest case fromtest_models.py. - After renaming
_format_duration: update any test references to the private name and ensurerender_live_sessions/render_full_summarystill renderRunningcolumn correctly (covered by existing tests). - After fixing
default_factory: no behaviour change; existing model-parsing tests should confirmtoolRequestsstill defaults to[].
Generated by Code Health Analysis · ◷