Skip to content

chore: extend ruff rule selection for the analytics package (#4942) - #4943

Open
hunterckx wants to merge 5 commits into
mainfrom
hunter/4942-extend-ruff-rules
Open

chore: extend ruff rule selection for the analytics package (#4942)#4943
hunterckx wants to merge 5 commits into
mainfrom
hunter/4942-extend-ruff-rules

Conversation

@hunterckx

Copy link
Copy Markdown
Contributor

(Text via Claude)

Closes #4942

What changed

Extends the ruff select list in analytics/pyproject.toml from ["E4", "E7", "E9", "F", "I", "W", "B"] to add C4, PIE, PTH, RET, RUF, SIM and UP, and fixes every violation the new categories surface.

One commit per rule category, each enabling the category and fixing its violations together, so each reviews independently and a regression traces back to one rule:

Commit Category Violations Change
1 C4, PIE, UP 0 Config only — no code touched
2 SIM 1 SIM108: if/else → ternary in api.py
3 RUF 2 RUF005 tuple unpacking in api.py; RUF022 sorted __all__ in static_site/__init__.py
4 RET 5 RET504 (assign-then-return) in _report_utils.py and api.py; 2× RET505 (else after return) in report_elements.py
5 PTH 33 os.path / os / globpathlib in static_site/export.py (26) and static_site/generator.py (7)

The PTH commit is the bulk of the work and the only real refactor. output_dir is normalized to a Path at each public entry point (generate_site and export_data), so the four generate_static_site.py scripts keep passing plain strings; os.makedirs becomes Path.mkdir(parents=True, exist_ok=True); the stale-detail-file sweep becomes Path.glob + Path.unlink; and every open(os.path.join(...)) becomes (output_dir / name).open(...).

Why

#4934 / #4936 added ruff to the analytics package with a deliberately minimal rule selection — enough to catch the star-import re-export leak that motivated it, but well short of what clevercanary/hca-validation-tools already runs on Python. Two Clever Canary Python codebases linting to two different standards means review habits don't transfer between them, and this package silently accumulates patterns that wouldn't survive review in the other repo.

Assumptions I made

  • E stays narrowed to E4/E7/E9 rather than taking all of E, which is the one intentional deviation from hca-validation-tools' selection. The non-preview rules full E would add over that subset are exactly E501 (line-too-long) and E101 (mixed-spaces-and-tabs) — both formatting concerns, and therefore ruff format's job. Keeping them out means the linter never duplicates or fights the formatter.
  • Scope is the select list only. hca-validation-tools' other lint settings (per-file-ignores, isort.known-first-party, line-length, target-version) are deliberately out of scope. No per-file-ignores entry was needed — every violation had a real fix, and no # noqa suppressions were added anywhere.
  • The PTH commit is path-handling only. A review pass suggested collapsing the ~10 near-identical openjson.dumpprint(f" Wrote …") blocks in export.py into a shared _write_json helper (net ≈ −16 lines). That repetition is pre-existing — this diff converted it, it didn't create it — so it was deliberately left alone to keep the commit reviewable against the rule it's named for. Filed as a follow-up instead.
  • output_dir accepting either str or Path was chosen over converting the four caller scripts. Normalizing at the library boundary keeps those scripts as declarative literals and avoids touching four out-of-scope files. The docstrings now state the accepted types, since every sibling argument in them already did.
  • Two cosmetic deltas in printed output are accepted, both stdout-only and neither touching generated site files:
    • os.path.abspath()Path.resolve() also resolves symlinks, so the final Files written to: … line may differ on a symlinked path.
    • Path("./site") stringifies as site, so anvil-catalog's closing cd ./site && … hint now prints cd site && ….

How to verify

The issue's definition of done, mapped to steps:

1. npm run lint:python and npm run check-format:python pass clean.

npm run lint:python        # expect: "All checks passed!"
npm run check-format:python # expect: "21 files already formatted"

2. The same steps pass in run-checks.yml CI. Check the analytics job on this PR. To reproduce its exact sequence locally:

cd analytics
uv run --locked --only-group dev ruff check .
uv run --locked --only-group dev ruff format --check .
uv sync --locked
uv run --no-sync python -c "import analytics.static_site"

3. Fresh-venv generate_static_site.py run including historic_data_path, with generated site output confirmed unchanged (per the #4913 verification convention). PTH rewrites path construction and file I/O in export.py and generator.py, so this checks output equality, not just importability:

  • Make GA4 credentials available per analytics/readme.md.credentials/hca_ga4_credentials.json for LungMAP.
  • Set CURRENT_MONTH in analytics/lungmap/constants.py to the month you want.
  • From analytics/lungmap, run uv run python generate_static_site.py and complete the browser OAuth flow. LungMAP is the right app here because it passes historic_data_path (HISTORIC_UA_DATA_PATH) and writes into gh-pages/lungmap, exercising the real output path.
  • Diff the generated gh-pages/lungmap tree against the same run on main. Expected: every file byte-identical except data/meta.json's generated_at timestamp.
  • Optionally serve it: cd gh-pages && python -m http.server 8080, then open http://localhost:8080.

Verification already performed:

  • The full CI sequence above passes locally after each of the 5 commits, and each commit went through the repo's pre-commit hook (prettier, eslint, tsc, ruff).
  • Step 3 has been run against LungMAP with historic_data_path: only generated_at differs.
  • Independently, a fixture harness drove generate_site (with fetch_data stubbed to canned data, including historic_data_path) and export_data directly, then fingerprinted every output file. Output is byte-identical between main at e738381 and this branch across all 13 JSON files plus index.html, with only meta.json's generated_at normalized. The harness also covers the two behavior-sensitive spots: mkdir(parents=True) creating a missing intermediate directory, and the glob/unlink sweep removing a stale event_*_detail.json while leaving a neighbouring file untouched. Both match main.

hunterckx and others added 5 commits August 22, 2026 18:13
Adds the flake8-comprehensions, flake8-pie and pyupgrade rule
categories to the analytics ruff selection, bringing it closer to the
selection used in clevercanary/hca-validation-tools. All three are
already clean, so this commit changes no code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the flake8-simplify rule category to the analytics ruff selection
and collapses the one `if`/`else` block it flags into a ternary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the ruff-specific rule category to the analytics ruff selection,
replacing a single-element tuple concatenation with unpacking and
sorting the `static_site` package's `__all__`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the flake8-return rule category to the analytics ruff selection,
returning expressions directly instead of assigning them first and
dropping two `else` branches that follow a `return`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the flake8-use-pathlib rule category to the analytics ruff
selection and moves the static site's path handling and file I/O from
`os.path`, `os` and `glob` to `pathlib`.

`output_dir` is normalized to a `Path` where it enters `generate_site`
and `export_data`, so callers can keep passing strings. `os.makedirs`
becomes `Path.mkdir(parents=True, ...)` to preserve parent creation, and
the absolute path printed at the end of a run now comes from
`Path.resolve()`, which additionally resolves symlinks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Extends Ruff coverage for the analytics package and applies lint-driven refactors, including migration of static-site file handling to pathlib.

Changes:

  • Adds seven Ruff rule categories.
  • Simplifies control flow, returns, tuple construction, and exports.
  • Refactors static-site path and file operations to use pathlib.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Summary
analytics/pyproject.toml Expands Ruff rule selection.
analytics/analytics_package/analytics/static_site/generator.py Uses Path for site generation paths.
analytics/analytics_package/analytics/static_site/export.py Uses Path for exports and cleanup.
analytics/analytics_package/analytics/static_site/__init__.py Sorts __all__.
analytics/analytics_package/analytics/report_elements.py Removes redundant else branches.
analytics/analytics_package/analytics/api.py Applies tuple, conditional, and return simplifications.
analytics/analytics_package/analytics/_report_utils.py Returns computed expressions directly.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

Extend ruff rule selection for the analytics package

3 participants