Restore in-progress books#69
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 0–1 of the refactoring study (
docs/refactoring-study.md) — the safe, no-behavioral-change slice. The goal of this PR is to fix the one concrete crash bug, lay a test safety net over the previously-untested data-access layer, and land low-risk hygiene/dedup, without changing any runtime/IO/exit/logging contract. The riskier behavioral items are deliberately deferred (see below) because this was executed unattended and they need human sign-off.Scope was reviewed by an external model (plan review) which recommended exactly this conservative partition for an unattended run.
What's in this PR
pytest-cov(dev dep +[tool.coverage]+ CI--covreporting). No--cov-fail-undergate.tests/conftest.py+ characterization tests pinning currentAPIClientbehavior (URL handling, success path, and the existing error-swallowing/dummy-token behavior)params=kwarg toAPIClient.get.API.get_device_data/get_weather_dataalready callclient.get(path, params=...), which raisedTypeErroron every device-data/weather fetch. Covered by new regression tests.asserts (measured.py,pipeline.py) with explicitraiseguards —assertis stripped underpython -Oand violates the repo's own ast-grep rule. Behavior-equivalent under normal execution.requests2.31.0 → >=2.32.4(CVE-2024-35195, CVE-2024-47081); drop redundantmatplotlib/weasyprintfrom the dev group._quarter_label(duplicated inpipeline.pyandbundle.py) into a singleconfig.quarter_label.Verification
uv run pytest→ 161 passed, 2 deselected (was 150; +11 new tests, no regressions).uv run pre-commit run --all-files→ ruff check, ruff format, and ast-grep all pass.src/api;src/utils/data_joiner.pyandtimeseries_generator.pyremain ~0–2% — see deferred work).Deliberately deferred — needs human sign-off
These were identified and intentionally NOT done because they change runtime behavior in zero-coverage areas where production env/consumer assumptions can't be verified unattended. Each is documented in
docs/refactoring-study.md:authenticate()/get()raise instead of returning a dummy token /{"data": []}. ~20 consumers assume a dict and never catch; no mid-run degrade path exists today. The characterization tests in this PR pin the current behavior precisely so this change can be made safely as a reviewed follow-up.config.py/dps_config.pyswitches DPS credentials from''→admin/password; committed Argo manifests don't injectAPI_USERNAME/API_PASSWORD, so DPS may currently run silently offline. Needs prod-env confirmation.type=boolflag — fix requires editing live Argo CronWorkflow manifests in lockstep.--unsafe-fixes— repo-wide implicit-Optional autofix is too much unreviewed annotation churn for an unattended run.CatalogService,SiteReport, …), logging-bootstrap unification: each touches import-time timing, operational log/stdout contracts, or needs a repo-wide "is this really dead / imported-by-name?" audit.Reference
Full analysis, target architecture, and prioritized roadmap:
docs/refactoring-study.md.