follow-up(statistics): P3 cleanups from PR #138 review #143

New issue

Closed

opened 2026-09-26 21:19:40 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-26 21:19:40 +00:00

Owner

Summary of P3 Cleanups from PR #138 Review Pass 2

During the second code review pass for PR #138 (feat/deck-marker-inputs, addressing Issue #129), all functional and architectural requirements were verified and all 416 tests pass. The following minor cleanup opportunities (P3) were identified:

1. Schema Default Consistency in `PeriodQuality`

In app/schemas/statistics.py:
closed_days: int = 0 carries a default, while the new tier counters (missing_days: int, unverified_days: int, estimate_days: int, unreliable_days: int) are defined without defaults after closed_days. Adding = 0 defaults to all tier counts ensures uniform partial construction and serialization defaults.

2. Domain Modeling for `DayQualityState`

In app/services/analytics_service.py:186:

DayQualityState = Literal["EXCLUDED", "MISSING", "UNVERIFIED", "ESTIMATED", "OK"]
ESTIMATE_STATES: frozenset[DayQualityState] = frozenset({"UNVERIFIED", "ESTIMATED"})

Currently modeled as a type alias and frozenset within the service. If other components (such as future export or reporting modules) interact with marker tiers, consider promoting DayQualityState to a StrEnum in app/schemas/statistics.py.

3. Shared Period Quality Test Harness Fixture

In tests/test_statistics_marker_inputs.py:76-81 and tests/test_cycle_verdict.py:128-134:
Both test suites define private period_quality(granularity, ...) helper functions constructing ScheduleCalendar. These can eventually be extracted to tests/conftest.py with typed Granularity literals.

Maintainer triage — 2026-09-27

Proceed with tier-counter default consistency, verifying existing response and construction behavior remains correct. Promote DayQualityState to a shared enum only when an actual second consumer justifies it; otherwise retain the current single service owner. Extract a shared test fixture only if it materially reduces duplication. Do not add abstractions solely for speculative reuse.

### Summary of P3 Cleanups from PR #138 Review Pass 2 During the second code review pass for PR #138 (`feat/deck-marker-inputs`, addressing Issue #129), all functional and architectural requirements were verified and all 416 tests pass. The following minor cleanup opportunities (P3) were identified: #### 1. Schema Default Consistency in `PeriodQuality` In `app/schemas/statistics.py`: `closed_days: int = 0` carries a default, while the new tier counters (`missing_days: int`, `unverified_days: int`, `estimate_days: int`, `unreliable_days: int`) are defined without defaults after `closed_days`. Adding `= 0` defaults to all tier counts ensures uniform partial construction and serialization defaults. #### 2. Domain Modeling for `DayQualityState` In `app/services/analytics_service.py:186`: ```python DayQualityState = Literal["EXCLUDED", "MISSING", "UNVERIFIED", "ESTIMATED", "OK"] ESTIMATE_STATES: frozenset[DayQualityState] = frozenset({"UNVERIFIED", "ESTIMATED"}) ``` Currently modeled as a type alias and frozenset within the service. If other components (such as future export or reporting modules) interact with marker tiers, consider promoting `DayQualityState` to a `StrEnum` in `app/schemas/statistics.py`. #### 3. Shared Period Quality Test Harness Fixture In `tests/test_statistics_marker_inputs.py:76-81` and `tests/test_cycle_verdict.py:128-134`: Both test suites define private `period_quality(granularity, ...)` helper functions constructing `ScheduleCalendar`. These can eventually be extracted to `tests/conftest.py` with typed `Granularity` literals. ## Maintainer triage — 2026-09-27 Proceed with tier-counter default consistency, verifying existing response and construction behavior remains correct. Promote DayQualityState to a shared enum only when an actual second consumer justifies it; otherwise retain the current single service owner. Extract a shared test fixture only if it materially reduces duplication. Do not add abstractions solely for speculative reuse.

gabogg added the

needs-triage

label

2026-09-26 21:19:40 +00:00

gabogg referenced this issue

2026-09-26 21:19:49 +00:00

feat(statistics): data-quality marker inputs for the deck #138

gabogg added

ready-for-agent

low-priority

and removed

needs-triage