-
Notifications
You must be signed in to change notification settings - Fork 2
👷 build: update nox #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
=======================================
Coverage ? 95.34%
=======================================
Files ? 2
Lines ? 43
Branches ? 0
=======================================
Hits ? 41
Misses ? 2
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the nox build and testing infrastructure by migrating from manual uv synchronization to nox-uv integration, streamlining CI workflows, and reorganizing project configuration.
Key Changes
- Migrates noxfile.py from standard nox sessions with manual
uv synccalls to nox-uv's@sessiondecorator with automatic dependency management viauv_groups - Consolidates CI workflow commands to use nox sessions consistently (format job now uses
nox -s lint, tests job usesnox -s test) - Adds
nox-uv,mypy, andbuildpackages to appropriate dependency groups; reorganizes pyproject.toml configuration sections alphabetically
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds build dependency group, includes nox-uv>=0.6.3 and mypy>=1.19.0 in respective groups, adds [tool.codespell] configuration, reorganizes tool sections alphabetically |
| noxfile.py | Migrates to nox-uv with @session decorator, removes manual uv sync boilerplate, renames tests session to test, simplifies parameter names to s, refactors build cleanup to use function call |
| .pre-commit-config.yaml | Migrates codespell skip configuration from inline args to pyproject.toml reference |
| .github/workflows/ci.yml | Renames pre-commit job to format, consolidates linting via nox, updates to setup-uv@v7.1.2 with commit hash in tests job, uses --no-default-groups for targeted dependency installation |
| .github/workflows/cd.yml | Pins hynek/build-and-inspect-python-package action to specific commit hash, adds persist-credentials: false |
|
|
||
| @nox.session(venv_backend="uv") | ||
| def tests(session: nox.Session, /) -> None: | ||
| @session(uv_groups=["test"], reuse_venv=True) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test session uses s.notify() to delegate to the pytest session but doesn't need the uv_groups=["test"] parameter since it only notifies and doesn't run any commands itself. This causes unnecessary dependency installation overhead.
| @session(uv_groups=["test"], reuse_venv=True) | |
| @session(reuse_venv=True) |
| build_path = DIR.joinpath("build") | ||
| if build_path.exists(): | ||
| shutil.rmtree(build_path) | ||
| rm_build(s) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build session calls rm_build(s) directly as a Python function, but rm_build is decorated with @session which makes it a nox session. This mixing of session invocation patterns is inconsistent. Either use s.notify("rm_build") to invoke it as a session, or remove the @session decorator from rm_build and make it a plain helper function.
| rm_build(s) | |
| s.notify("rm_build") |
| steps: | ||
| - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v6 |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format job uses astral-sh/setup-uv@v6 (unpinned) while the tests job uses astral-sh/setup-uv@85856786d1ce8acfbcc2f13a5f3fbd6b938f9f41 # v7.1.2 (pinned with commit hash). These should be consistent - either both should use pinned versions with commit hashes for security and reproducibility, or both should use version tags. The recommended practice is to pin both with commit hashes.
| uses: astral-sh/setup-uv@v6 | |
| uses: astral-sh/setup-uv@85856786d1ce8acfbcc2f13a5f3fbd6b938f9f41 # v7.1.2 |
| dev = [ | ||
| "ipykernel>=6.29.5", | ||
| "cz-conventional-gitmoji>=0.6.1", | ||
| {include-group = "build"}, |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The build dependency group is included in the dev group (line 49), and the build session in noxfile.py uses uv_groups=["build"]. However, the build package is typically only needed for building distributions, not for general development. Consider whether build should be in the dev group or only installed when explicitly needed via the build session.
| {include-group = "build"}, |
|
|
||
| @nox.session(venv_backend="uv") | ||
| def lint(session: nox.Session, /) -> None: | ||
| @session(uv_groups=["lint"], reuse_venv=True) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint session uses s.notify() to call other sessions, but it doesn't actually need the uv_groups=["lint"] parameter since it only delegates to other sessions and doesn't run any commands itself. The uv_groups parameter installs dependencies, which is unnecessary overhead for a session that only notifies other sessions.
| @session(uv_groups=["lint"], reuse_venv=True) | |
| @session(reuse_venv=True) |
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
No description provided.