Merged
Conversation
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Co-Authored-By: Llewellyn Falco <llewellyn.falco@gmail.com>
Reviewer's GuideMigrates the project from directly invoking setup.py to using PEP 517 Sequence diagram for the new publish script build processsequenceDiagram
actor Maintainer
participant Shell
participant Publish_script as publish_approvaltests_sh
participant Setup_py_shim as setup_py
participant Build_module as python_m_build
participant Setuptools_backend as setuptools_build_meta
participant Dist_dir as dist_directory
participant Twine
Maintainer->>Shell: run publish_approvaltests_sh
Shell->>Publish_script: execute script
Publish_script->>Publish_script: copy setup_setup_publish_py to setup_py
Publish_script->>Build_module: python -m build .
Build_module->>Setuptools_backend: build_sdist_and_wheel
Setuptools_backend->>Setup_py_shim: import setup_utils and call do_the_setup
Setup_py_shim->>Setuptools_backend: provide package_metadata_and_config
Setuptools_backend->>Dist_dir: write wheel_and_sdist_files
Publish_script->>Publish_script: remove setup_py
Publish_script->>Twine: twine upload dist_files
Twine->>Dist_dir: read built_artifacts
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}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the shell publish scripts, consider using
trapto ensuresetup.pyis removed even ifpython -m build .ortwine uploadfails, so you don’t leave a straysetup.pyin the repo on error. - The
_setup_dirlogic insetup.setup.publish.pyandsetup.setup.minimal.pyusesPath(__file__).resolve().parent / "setup", which for files already insetup/points tosetup/setup; ifsetup_utils.pylives directly insetup/, you can simplify this toPath(__file__).resolve().parent(or drop thesys.pathmanipulation entirely) to avoid confusion and potential path issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the shell publish scripts, consider using `trap` to ensure `setup.py` is removed even if `python -m build .` or `twine upload` fails, so you don’t leave a stray `setup.py` in the repo on error.
- The `_setup_dir` logic in `setup.setup.publish.py` and `setup.setup.minimal.py` uses `Path(__file__).resolve().parent / "setup"`, which for files already in `setup/` points to `setup/setup`; if `setup_utils.py` lives directly in `setup/`, you can simplify this to `Path(__file__).resolve().parent` (or drop the `sys.path` manipulation entirely) to avoid confusion and potential path issues.
## Individual Comments
### Comment 1
<location path="tidy_code.sh" line_range="5" />
<code_context>
set -euo pipefail
mise task --quiet run tidy_code
+git add --renormalize .
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Be cautious about staging all files from within a tidy script.
`git add --renormalize .` will stage *all* tracked files, not just those affected by this tidy run, which can lead to accidentally committing unrelated changes (e.g., widespread line-ending updates). If you only want to normalize a subset of paths, consider limiting the command to specific directories or omitting `git add` here and leaving staging to the caller.
</issue_to_address>
### Comment 2
<location path="test__mypy_accepts_our_packages.py" line_range="27-28" />
<code_context>
+ finally:
+ _unlink_with_retry(pathlib.Path("setup.py"))
+
+ wheel_files = glob.glob("dist/*.whl")
+ assert len(wheel_files) == 1, f"Expected 1 wheel, found {wheel_files}"
_run_python_checked(
[
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the wheel belongs to the expected package, not just that there is exactly one wheel.
As written, this will pass as long as there’s exactly one wheel in `dist/`, even if it’s for a different package. To make the test safer, also assert that the wheel filename matches the expected package name, e.g. `Path(wheel_files[0]).name.startswith(f"{package_name}-")`, so mismatches between the built wheel and the package under test are caught.
</issue_to_address>
### Comment 3
<location path="test__mypy_accepts_our_packages.py" line_range="52-61" />
<code_context>
def _run_python_checked(
- args: typing.List[str], cwd: typing.Optional[pathlib.Path] = None
+ args: typing.List[str],
+ cwd: typing.Optional[pathlib.Path] = None,
+ quiet: bool = False,
) -> None:
subprocess.run(
[sys.executable, *args],
check=True,
cwd=cwd,
+ stdout=subprocess.DEVNULL if quiet else None,
+ stderr=subprocess.DEVNULL if quiet else None,
)
</code_context>
<issue_to_address>
**suggestion:** Quiet mode hides build output, which can make diagnosing failing packaging tests difficult.
Using `quiet=True` here suppresses both stdout and stderr, so if the build fails in CI you lose the error details needed to debug packaging issues. Consider keeping stderr visible (or redirecting it to a log file) when `quiet` is enabled, or only applying `quiet` to the subsequent `pip install` step so failures remain diagnosable.
Suggested implementation:
```python
# Suppress only stdout in quiet mode so build / packaging errors remain visible
stdout=subprocess.DEVNULL if quiet else None,
stderr=None,
```
If any callers relied on stderr being suppressed by `quiet=True`, they may need to be updated to handle visible error output now (for example, by explicitly redirecting or capturing stderr at the call site if necessary).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| set -euo pipefail | ||
|
|
||
| mise task --quiet run tidy_code | ||
| git add --renormalize . |
There was a problem hiding this comment.
suggestion (bug_risk): Be cautious about staging all files from within a tidy script.
git add --renormalize . will stage all tracked files, not just those affected by this tidy run, which can lead to accidentally committing unrelated changes (e.g., widespread line-ending updates). If you only want to normalize a subset of paths, consider limiting the command to specific directories or omitting git add here and leaving staging to the caller.
Comment on lines
+27
to
+28
| wheel_files = glob.glob("dist/*.whl") | ||
| assert len(wheel_files) == 1, f"Expected 1 wheel, found {wheel_files}" |
There was a problem hiding this comment.
suggestion (testing): Also assert that the wheel belongs to the expected package, not just that there is exactly one wheel.
As written, this will pass as long as there’s exactly one wheel in dist/, even if it’s for a different package. To make the test safer, also assert that the wheel filename matches the expected package name, e.g. Path(wheel_files[0]).name.startswith(f"{package_name}-"), so mismatches between the built wheel and the package under test are caught.
Comment on lines
52
to
+61
| def _run_python_checked( | ||
| args: typing.List[str], cwd: typing.Optional[pathlib.Path] = None | ||
| args: typing.List[str], | ||
| cwd: typing.Optional[pathlib.Path] = None, | ||
| quiet: bool = False, | ||
| ) -> None: | ||
| subprocess.run( | ||
| [sys.executable, *args], | ||
| check=True, | ||
| cwd=cwd, | ||
| stdout=subprocess.DEVNULL if quiet else None, |
There was a problem hiding this comment.
suggestion: Quiet mode hides build output, which can make diagnosing failing packaging tests difficult.
Using quiet=True here suppresses both stdout and stderr, so if the build fails in CI you lose the error details needed to debug packaging issues. Consider keeping stderr visible (or redirecting it to a log file) when quiet is enabled, or only applying quiet to the subsequent pip install step so failures remain diagnosable.
Suggested implementation:
# Suppress only stdout in quiet mode so build / packaging errors remain visible
stdout=subprocess.DEVNULL if quiet else None,
stderr=None,If any callers relied on stderr being suppressed by quiet=True, they may need to be updated to handle visible error output now (for example, by explicitly redirecting or capturing stderr at the call site if necessary).
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
Switch packaging and publishing workflows from direct setup.py invocation to PEP 517/518 build tooling and adjust tests and config accordingly.
Enhancements:
Build:
CI:
Tests:
Chores: