fix: code quality cleanups in report.py and models.py (#66)#93
fix: code quality cleanups in report.py and models.py (#66)#93
Conversation
- Extract _format_session_running_time helper to deduplicate running-time computation in render_live_sessions and _render_active_section - Rename shadowed sessions parameter to filtered in render_cost_view - Standardize _render_totals output-token summation to comprehension form - Simplify AssistantMessageData.toolRequests default_factory to list - Add unit tests for _format_session_running_time Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses several small code-quality issues in the Copilot usage reporting and telemetry models, primarily by deduplicating logic, avoiding parameter shadowing, and aligning implementation style across similar code paths.
Changes:
- Extracted
_format_session_running_time()and reused it in both live-session and active-section renderers. - Avoided shadowing
sessionsinrender_cost_view()by introducing afilteredvariable. - Standardized total output-token summation in
_render_totals()to use a generator-expression form. - Simplified
AssistantMessageData.toolRequeststo useField(default_factory=list)and added unit tests for the new helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/copilot_usage/report.py |
Refactors duplicated “running time” formatting into a helper; minor cleanups in totals and cost view. |
src/copilot_usage/models.py |
Simplifies toolRequests default factory to match project-wide Pydantic collection conventions. |
tests/copilot_usage/test_report.py |
Adds unit tests covering _format_session_running_time behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #66 by performing small code-quality cleanups in the Copilot usage reporting code and related Pydantic models, plus adding focused unit tests for the extracted helper.
Changes:
- Extracts
_format_session_running_timeto de-duplicate running-time formatting logic and adds unit tests for it. - Avoids parameter shadowing in
render_cost_viewby using afilteredvariable. - Standardizes output-token summation style in
_render_totalsand simplifies a Pydanticdefault_factoryfortoolRequests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/copilot_usage/report.py |
Extracts running-time helper, cleans up cost-view variable naming, and standardizes token aggregation. |
src/copilot_usage/models.py |
Simplifies toolRequests default factory to list (but introduces a questionable pyright ignore). |
tests/copilot_usage/test_report.py |
Adds unit tests covering _format_session_running_time behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quality Gate — AUTO-APPROVED
Evaluation summary:
- ✅
awlabel present - ✅ CI (
check) passing - ✅ Copilot auto-review completed (COMMENTED)
Code quality: Good
- Clean extraction of
_format_session_running_timehelper eliminating duplication in two call sites - Variable rename in
render_cost_viewcorrectly avoids parameter shadowing - Token summation refactored to generator expression matching existing project style
toolRequestsdefault_factory simplified to match project Pydantic convention (Field(default_factory=list))- 3 focused unit tests covering all branches of the new helper
Impact: LOW — Purely internal refactoring (helper extraction, variable rename, style alignment) with no changes to public API, data models, or business logic. All existing tests pass unmodified.
Note: Copilot reviewer flagged the # pyright: ignore[reportUnknownVariableType] on toolRequests as potentially unnecessary/inconsistent. This is a minor style nit, not a correctness issue — it can be addressed in a follow-up if desired.
Auto-approving for merge.
|
Commit pushed:
|
Remove pyright ignore comment on toolRequests field by using a typed factory (list[dict[str, object]]) instead of bare list. This gives pyright the type information it needs without suppressing diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Closing — will re-dispatch implementer after pipeline fixes land (PR #97). |
Pull request was closed
microsasa
deleted the
fix/issue-66-code-quality-cleanups-92a4ff0a9a4e52c6
branch
Closes #66
Changes
Extract
_format_session_running_timehelper — The duplicated 4-line running-time expression inrender_live_sessionsand_render_active_sectionis now a single private helper.Rename shadowed parameter in
render_cost_view— Thesessionsparameter was being reassigned to a filtered result. Renamed tofilteredto match the pattern used elsewhere and avoid shadowing confusion.Standardize output-token summation in
_render_totals— Replaced the nestedforloop with the same generator-expression style used in_render_historical_section.Simplify
AssistantMessageData.toolRequestsfactory — Changed fromlambda: list[dict[str, object]]()(misleading runtime-erased generic) todefault_factory=list, consistent with all other collection fields.Testing
_format_session_running_time: nostart_time→ dash,start_timeonly, andlast_resume_timepreferred overstart_time.