Cleanup from UV migration + standardization of workflows and package config#336
Conversation
4d3b79a to
6d628c3
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean tooling modernisation — migrating test/dev/docs to PEP 735 [dependency-groups], upgrading pre-commit hooks, and improving CI hygiene. All 28 CI checks pass.
No UI files changed; Phase 3 skipped.
Two nitpicks: a pair of redundant step-level skip conditions that the refactor missed cleaning up in check_migrations_sqlite.yml, and inconsistent action-version pin style in the new pre-commit.yml. Everything else looks solid.
backend review: This PR is a clean, mechanical refactor: the vast majority of changes are import statement reformatting enforced by ruff's new force-single-line = true isort setting, applied consistently to all ~80 Python files. No Python backend logic was changed — every diff in morango/ and tests/ is either a grouped-import split or a trivial formatting tweak (e.g. _self_ref_order field definition line-length). The .git-blame-ignore-revs entry correctly marks the reformatting commit.
The more substantive changes are in the build and CI layer: test/dev/docs groups migrate from [project.optional-dependencies] to PEP 735 [dependency-groups], which is the right separation (they're internal tooling deps, not published package extras). postgres and accelerated correctly remain as [project.optional-dependencies]. The dev group gains {include-group = "test"}, making it a proper superset. CI workflows gain concurrency-cancel blocks and move per-step if: skip_check guards to the job level, which is cleaner. EOL Python matrix entries gain --sitepackages + uv pip install --system --group test to work around uv's limitations with Python < 3.8.
Two minor issues found: an unpinned third-party action in the new pre-commit workflow, and two leftover step-level if conditions in check_migrations_sqlite.yml that are now redundant.
[nitpick] Redundant step-level skip conditions in migration check workflow
The migration_test job in check_migrations_sqlite.yml now has a job-level if: ${{ needs.pre_job.outputs.should_skip != 'true' }} (added by this PR), which means the job is skipped entirely when should_skip is true. Two step-level conditions that pre-date this PR were not cleaned up:
- "Install build dependencies" step (line 55) still has
if: ${{ needs.pre_job.outputs.should_skip != 'true' }} - "Run migrations" step (line 99) still has the same condition
These are now no-ops — the job never reaches them when skipping. All other workflows had their step-level conditions promoted to the job level correctly; these two were missed. Safe to remove both.
[praise] EOL Python workaround is well-crafted
The approach for Python < 3.9 (tox < 4, which doesn't support dependency_groups) — pre-installing test deps into the system Python via uv pip install --system --group test and then passing --sitepackages to tox — is a clean, minimal workaround with no footguns. It avoids duplicating the dependency list and stays consistent with how modern Python versions get their deps. The same pattern is applied uniformly across EOL unit tests, EOL cryptography tests, Windows, and migration checks, and CI confirms it works across Python 3.6–3.8.
[praise] Skip-condition refactor improves CI clarity
Moving if: ${{ needs.pre_job.outputs.should_skip != 'true' }} from every individual step up to the job level is the correct pattern: it prevents GitHub from even starting the job (saving runner minutes and reducing log noise) rather than running the job and skipping each step individually. The change is applied consistently across tox.yml, check_migrations_sqlite.yml, and check-docs.yml.
[praise] git-blame-ignore-revs housekeeping
Adding the import-reformatting commit to .git-blame-ignore-revs is good housekeeping — git blame --ignore-revs-file .git-blame-ignore-revs (and GitHub's blame UI, which respects this file) will skip over the mechanical reformatting and show the original authors.
[praise] PEP 735 dependency groups migration is correct
test, dev, and docs moving from [project.optional-dependencies] to [dependency-groups] is the right call: these are internal tooling deps that shouldn't be exposed as published package extras, and uv's native group support is the proper mechanism. postgres and accelerated correctly remain as optional-dependencies since they change runtime behaviour for consumers. The {include-group = "test"} entry in dev is a clean DRY improvement.
[praise] Import reformatting handled cleanly end-to-end
The force-single-line = true isort rule is introduced in pyproject.toml, applied mechanically by ruff, and the reformatting commit is registered in .git-blame-ignore-revs. Each import is now independently navigable in IDEs and diffs are more granular. The scope is complete — all Python files in the diff follow the new style.
Comments on lines not in diff:
.github/workflows/check_migrations_sqlite.yml:55 — [nitpick] Redundant step-level if conditions in check_migrations_sqlite.yml
The migration_test job gained a job-level if: ${{ needs.pre_job.outputs.should_skip != 'true' }} in this PR (line 49), which correctly prevents the entire job from running when the skip check fires. However, two step-level copies of the same condition were not removed:
- Line 55:
Install build dependenciesstep - Line 99:
Run migrationsstep
All other equivalent step-level conditions were removed in this PR. These two are now dead code — the job won't reach them when should_skip == 'true'. Low impact, but inconsistent with the rest of the cleanup.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Discarded findings citing files outside the diff; checked CI status and linked issue acceptance criteria
- Calibrated the final verdict deterministically from finding severities, CI status, and QA evidence
rtibbles
left a comment
There was a problem hiding this comment.
Just one question - I didn't read the reformatting commit to save my brain, just assuming that ruff works as expected :)
Summary
pyproject.toml, makingdevandtestdependency groups instead of optional, leavingpostgresandacceleratedas optionaluvto install the group deps into the system packages, then allows tox to use thoseReviewer guidance
AI Usage
Had AI start the process, then wrapped it up myself, as there were challenges with python+tox+pip versioning and other stuff was just copy & paste.