Rename pypi project to nixie cli - #31
Merged
Merged
Conversation
Switch setuptools to package discovery with an explicit exclusion for `nixie.unittests`, so wheel builds no longer warn about the importable subpackage and do not publish the unit test modules. Extend `make clean` to remove `build/`, which prevents stale build artefacts from leaking old test files into later wheels. Add packaging regression tests that build a throwaway wheel copy and verify that `nixie.unittests` stays out of the artefact.
Publish the package as `nixie-cli` while keeping the installed console script as `nixie`. Update the packaging regression test to assert both the wheel metadata and the preserved console entry point, and refresh the user-facing installation docs to match the new PyPI name.
Update the end-user installation guidance to recommend `uv tool install nixie-cli`. This keeps the README aligned with the new PyPI distribution name and the preferred tool-install workflow for a CLI package.
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRename the PyPI distribution to Changes
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
leynos
marked this pull request as ready for review
April 18, 2026 14:07
Contributor
Reviewer's GuideRenames the published PyPI distribution to Flow diagram for packaging configuration and clean behaviourflowchart TD
A[pyproject.toml project.name = nixie-cli] --> B[setuptools.find_packages<br>include = nixie<br>exclude = nixie.unittests and nixie.unittests.*<br>namespaces = false]
B --> C[Build wheel distribution]
C --> D[Wheel metadata shows distribution name nixie-cli]
C --> E[Wheel contents include nixie package only]
E --> F[Unit test modules are excluded from wheel]
D --> G[uv tool install nixie-cli]
G --> H[Environment installs nixie-cli distribution]
H --> I[Entry point nixie = nixie.cli:cli]
I --> J[nixie console command available to user]
subgraph Makefile_clean_target
K[make clean] --> L[Remove .venv]
K --> M[Remove build directory]
K --> N[Remove dist directory]
K --> O[Remove *.egg-info]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Skip the packaging integration tests when `uv` or `make` are not installed instead of hard-failing the suite. Also assert that the stale build artefact exists before running `make clean` so the test proves the cleanup removed a real file.
Owner
Author
|
@coderabbitai Have the following now been resolved? Please address the comments from this code review: Overall Comments
Individual CommentsComment 1+def test_make_clean_removes_the_build_directory(tmp_path: Path) -> None: + """Ensure ``make clean`` clears stale build artefacts before packaging.""" + build_root = _copy_packaging_fixture(tmp_path, include_makefile=True) + stale_test_file = build_root / "build/lib/nixie/unittests/stale_test.py" + stale_test_file.parent.mkdir(parents=True, exist_ok=True) + stale_test_file.write_text("pass\n", encoding="utf-8") + + make_executable = shutil.which("make") **suggestion (testing):** Assert that the stale file exists before running `make clean` to prove the behaviour under testThe test currently relies on the stale file being created but never verifies it. Please add </issue_to_address> |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3758
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_packaging.py`:
- Around line 54-68: The test currently uses next() without defaults which
raises StopIteration and yields unhelpful tracebacks; change the three next()
usages to provide a default (e.g. next(..., None)) and then assert the result is
not None with a clear failure message. Concretely, for wheel_path, use
wheel_path = next((build_root / "dist").glob("*.whl"), None) and assert
wheel_path is not None with "wheel file not found", and for metadata_name and
entry_points_name use next(packaged_file_generator, None) then assert
metadata_name is not None with "METADATA file not found in wheel" and similarly
assert entry_points_name is not None with "entry_points.txt not found in wheel";
this preserves packaged_files usage and only updates the three identifiers
wheel_path, metadata_name, and entry_points_name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 328de637-978f-4ef6-9749-29ebc4b82b5b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
MakefileREADME.mddocs/CHANGELOG.mdpyproject.tomltests/integration/test_packaging.py
Replace bare `next()` calls in the packaging integration test with `next(..., None)` plus explicit assertions. This keeps the test behaviour unchanged on the happy path while producing clearer failures when the wheel or wheel metadata is missing.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add explicit timeouts to the packaging integration test subprocess calls so CI cannot hang indefinitely. Also document the `S603` suppressions inline because both invocations use static argv in a controlled test environment with `shell=False`.
Move the packaging project copy logic into integration fixtures so the packaging tests receive prepared project roots instead of managing local setup helpers. Also add explicit failure messages to the remaining bare assertions in `test_packaging.py` so packaging mismatches are easier to diagnose.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/integration/test_packaging.py (2)
13-35:⚠️ Potential issue | 🟠 MajorReplace helper-led shared setup with pytest fixtures.
Move project-copy setup and executable provisioning into fixtures under
conftest.py, then inject them into test functions. Eliminate orchestration logic from test bodies.As per coding guidelines, "Use pytest fixtures for shared setup (
conftest.pyorfixtures/)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_packaging.py` around lines 13 - 35, The helper function _copy_packaging_fixture is being used for shared test setup instead of pytest fixtures; move its logic into one or more fixtures in conftest.py (e.g., a packaging_project or copy_packaging_fixture fixture) that create the build_root directory, copy the listed files (names = ["pyproject.toml","README.md","LICENSE","nixie"] and optionally "Makefile") and return the Path to the created project, and then update tests to accept that fixture instead of calling _copy_packaging_fixture directly; ensure the fixture encapsulates the include_makefile behavior (via parameterized fixtures or two fixtures) and preserves the use of shutil.copytree/copy2 for directories/files and use the same build_root naming.
92-93:⚠️ Potential issue | 🟡 MinorAdd explicit failure messages to all bare asserts.
Attach clear assertion messages so failures report expected and actual values immediately.
Diff proposal
- assert "Name: nixie-cli" in metadata - assert "nixie = nixie.cli:cli" in entry_points + assert "Name: nixie-cli" in metadata, ( + f"Expected 'Name: nixie-cli' in wheel METADATA, got: {metadata!r}" + ) + assert "nixie = nixie.cli:cli" in entry_points, ( + "Expected console entry point 'nixie = nixie.cli:cli' " + f"in entry_points.txt, got: {entry_points!r}" + ) @@ - assert unittest_entries == [] + assert unittest_entries == [], ( + f"Expected wheel to exclude nixie/unittests/, got: {unittest_entries!r}" + ) @@ - assert not (build_root / "build").exists() + assert not (build_root / "build").exists(), ( + "Expected `make clean` to remove build/ directory" + )As per coding guidelines, "Use
assert …, "message"over bare asserts".Also applies to: 100-100, 125-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_packaging.py` around lines 92 - 93, Replace bare asserts with assertions that include clear failure messages: change assertions like assert "Name: nixie-cli" in metadata and assert "nixie = nixie.cli:cli" in entry_points to include messages (e.g., assert "Name: nixie-cli" in metadata, f"expected package name 'Name: nixie-cli' in metadata; got: {metadata!r}" and assert "nixie = nixie.cli:cli" in entry_points, f"expected entry point 'nixie = nixie.cli:cli' in entry_points; got: {entry_points!r}"). Apply the same pattern to the other bare asserts in this test file (the other assertions referenced in the review) so failures print expected vs actual values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_packaging.py`:
- Around line 13-35: The helper function _copy_packaging_fixture is being used
for shared test setup instead of pytest fixtures; move its logic into one or
more fixtures in conftest.py (e.g., a packaging_project or
copy_packaging_fixture fixture) that create the build_root directory, copy the
listed files (names = ["pyproject.toml","README.md","LICENSE","nixie"] and
optionally "Makefile") and return the Path to the created project, and then
update tests to accept that fixture instead of calling _copy_packaging_fixture
directly; ensure the fixture encapsulates the include_makefile behavior (via
parameterized fixtures or two fixtures) and preserves the use of
shutil.copytree/copy2 for directories/files and use the same build_root naming.
- Around line 92-93: Replace bare asserts with assertions that include clear
failure messages: change assertions like assert "Name: nixie-cli" in metadata
and assert "nixie = nixie.cli:cli" in entry_points to include messages (e.g.,
assert "Name: nixie-cli" in metadata, f"expected package name 'Name: nixie-cli'
in metadata; got: {metadata!r}" and assert "nixie = nixie.cli:cli" in
entry_points, f"expected entry point 'nixie = nixie.cli:cli' in entry_points;
got: {entry_points!r}"). Apply the same pattern to the other bare asserts in
this test file (the other assertions referenced in the review) so failures print
expected vs actual values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80dbce35-bb3a-43ba-a7b8-0f8b0ba8d12d
📒 Files selected for processing (1)
tests/integration/test_packaging.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/conftest.py`:
- Around line 46-47: The test fixture currently hardcodes build_root =
destination_root / "package-copy" and calls build_root.mkdir(), which can
collide when multiple fixtures are requested; change to create a unique
directory per fixture by appending a unique suffix (e.g. uuid4 or a timestamp)
to "package-copy" or use a temporary unique directory API, or call
build_root.mkdir(exist_ok=True) combined with a unique name; update the code
referencing build_root (the build_root and destination_root variables) so each
fixture instance gets its own target directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3772af0-f286-4a89-a8fd-a7e1419f1ab1
📒 Files selected for processing (2)
tests/integration/conftest.pytests/integration/test_packaging.py
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the current state of the file to confirm. 🏁 Script executed: Length of output: 4167
No annulments apply: neither requirement touches spelling (the docstring already uses the correct British form "artefacts"), and neither increases cyclomatic complexity. |
Give each copied packaging project a unique target directory instead of always using `package-copy` under `tmp_path`. This avoids fixture collisions when multiple packaging fixtures are requested within the same test and preserves the existing copy logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/conftest.py`:
- Around line 50-52: The fixture currently builds a mutable list named "names"
and appends "Makefile" when include_makefile is true; change this to use an
immutable tuple by defining names = ("pyproject.toml", "README.md", "LICENSE",
"nixie") and, when include_makefile is true, extend via tuple concatenation
(e.g. names = names + ("Makefile",)); ensure any downstream usage that expects a
list either accepts a tuple or explicitly converts names to list at the call
site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96a66ab9-0a78-4a99-8805-0fa0cfcbc149
📒 Files selected for processing (1)
tests/integration/conftest.py
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
Replace the mutable `names` list in the packaging fixture helper with an immutable tuple. Keep the optional `Makefile` handling inside tuple construction so the fixture remains side-effect free and still copies the same files.
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
1 participant
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 by Sourcery
Rename the packaged PyPI distribution to
nixie-cliand tighten packaging so unit tests and stale build artifacts are excluded from published wheels.Enhancements:
Build:
nixie-cliin packaging metadata and ensuremake cleanremoves the build directory before packaging.Documentation:
nixie-cliwhile keeping thenixieCLI command name.Tests:
make clean.