Skip to content

Conversation

@nstarman
Copy link
Contributor

No description provided.

Signed-off-by: nstarman <nstarman@users.noreply.github.com>
@nstarman nstarman added this to the v0.2.x milestone Dec 10, 2025
Copilot AI review requested due to automatic review settings December 10, 2025 04:50
@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@ecbe9c8). Learn more about missing BASE report.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nstarman nstarman merged commit 286216b into GalacticDynamics:main Dec 10, 2025
18 checks passed
@nstarman nstarman deleted the nox branch December 10, 2025 04:53
meeseeksmachine pushed a commit to meeseeksmachine/xmmutablemap that referenced this pull request Dec 10, 2025
Copy link

Copilot AI left a 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 sync calls to nox-uv's @session decorator with automatic dependency management via uv_groups
  • Consolidates CI workflow commands to use nox sessions consistently (format job now uses nox -s lint, tests job uses nox -s test)
  • Adds nox-uv, mypy, and build packages 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)
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
@session(uv_groups=["test"], reuse_venv=True)
@session(reuse_venv=True)

Copilot uses AI. Check for mistakes.
build_path = DIR.joinpath("build")
if build_path.exists():
shutil.rmtree(build_path)
rm_build(s)
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
rm_build(s)
s.notify("rm_build")

Copilot uses AI. Check for mistakes.
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
- name: Install uv
uses: astral-sh/setup-uv@v6
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
uses: astral-sh/setup-uv@v6
uses: astral-sh/setup-uv@85856786d1ce8acfbcc2f13a5f3fbd6b938f9f41 # v7.1.2

Copilot uses AI. Check for mistakes.
dev = [
"ipykernel>=6.29.5",
"cz-conventional-gitmoji>=0.6.1",
{include-group = "build"},
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
{include-group = "build"},

Copilot uses AI. Check for mistakes.

@nox.session(venv_backend="uv")
def lint(session: nox.Session, /) -> None:
@session(uv_groups=["lint"], reuse_venv=True)
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
@session(uv_groups=["lint"], reuse_venv=True)
@session(reuse_venv=True)

Copilot uses AI. Check for mistakes.
nstarman added a commit that referenced this pull request Dec 10, 2025
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
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.

1 participant