Conversation
Reviewer's GuideModernizes the packaging and build configuration by moving metadata and dependencies into pyproject.toml using PEP 621, simplifying setup.py to only handle compiled extensions, adjusting test tooling configuration, and bumping the package version. Sequence diagram for building the package with modernized configurationsequenceDiagram
participant Dev as Developer
participant Pip as pip
participant SB as setuptools_build_meta
participant CY as Cython
participant SR as setuptools_rust
Dev->>Pip: pip wheel .
Pip->>SB: Read pyproject.toml
SB-->>Pip: Build requirements
Pip->>Pip: Install build deps
Pip->>SB: Invoke build_wheel
SB->>SB: Load project metadata
SB->>CY: cythonize extensions
CY-->>SB: Compiled C sources
SB->>SR: Build Rust extensions
SR-->>SB: Compiled Rust artifacts
SB-->>Pip: Built wheel
Pip-->>Dev: Wheel file in dist/
Flow diagram for new build and test workflow using pyproject.tomlflowchart TD
A[Start] --> B[Edit source code and datamodel.version]
B --> C[Define metadata and dependencies in pyproject.toml
project section]
C --> D[Configure build-system
setuptools.build_meta
and build requirements]
D --> E[Configure extensions in setup.py
Cython and Rust extensions only]
E --> F[Build package
python -m build / pip wheel]
F --> G[setuptools.build_meta reads pyproject.toml
and setup.py]
G --> H[Compile Cython extensions
via cythonize]
H --> I[Build Rust extensions
via setuptools_rust]
I --> J[Produce wheel and sdist]
J --> K[Run tests with pytest
using tool.pytest.ini_options]
K --> L[Optional: manage deps with uv
override setuptools-rust]
L --> M[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using
packages = ["datamodel"]in[tool.setuptools]will exclude nested subpackages (e.g.datamodel.rs_parsers,datamodel.libs.*); consider switching topackages = setuptools.find_namespace_packages(...)orfind_packages()configuration to ensure all modules are included in the wheel. - Adding
pip>=24.3.1to[build-system].requirescan cause unnecessary bootstrap/circular dependency issues in build environments; it’s usually better to rely on the frontend’s pip and keep only actual build backends and plugins in this list. - Now that you’ve centralized install requirements in
[project.dependencies], thesetup.pyno longer declaresinstall_requires; to avoid divergence if both build paths are used, consider either delegating entirely to pyproject-based builds or makingsetup.pyread from the same source of dependency metadata.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `packages = ["datamodel"]` in `[tool.setuptools]` will exclude nested subpackages (e.g. `datamodel.rs_parsers`, `datamodel.libs.*`); consider switching to `packages = setuptools.find_namespace_packages(...)` or `find_packages()` configuration to ensure all modules are included in the wheel.
- Adding `pip>=24.3.1` to `[build-system].requires` can cause unnecessary bootstrap/circular dependency issues in build environments; it’s usually better to rely on the frontend’s pip and keep only actual build backends and plugins in this list.
- Now that you’ve centralized install requirements in `[project.dependencies]`, the `setup.py` no longer declares `install_requires`; to avoid divergence if both build paths are used, consider either delegating entirely to pyproject-based builds or making `setup.py` read from the same source of dependency metadata.
## Individual Comments
### Comment 1
<location> `pyproject.toml:46` </location>
<code_context>
+dependencies = [
+ "numpy>=1.26.4",
+ "uvloop>=0.21.0",
+ "asyncio==3.4.3",
+ "faust-cchardet==2.1.19",
+ "ciso8601==2.3.2",
</code_context>
<issue_to_address>
**issue (bug_risk):** Pinning `asyncio` from PyPI conflicts with the stdlib `asyncio` on Python ≥3.10.
Given `requires-python = ">=3.10.0"`, `asyncio` should come from the standard library; the PyPI `asyncio` package is a legacy backport. Pinning `asyncio==3.4.3` risks shadowing the stdlib module and causing subtle runtime issues. Please remove this dependency and rely on the stdlib version, or, if the backport is truly required, document the rationale and ensure it cannot override the built-in module on supported platforms.
</issue_to_address>
### Comment 2
<location> `pyproject.toml:69` </location>
<code_context>
+
+[tool.setuptools]
+license-files = ["LICENSE"]
+packages = ["datamodel"]
+include-package-data = true
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Hardcoding `packages = ["datamodel"]` may omit nested subpackages that were previously auto-discovered.
Previously `find_namespace_packages` in `setup.py` auto-discovered all subpackages under `datamodel` (e.g. `datamodel.rs_parsers`, `datamodel.libs.*`). With `packages = ["datamodel"]`, those subpackages may no longer be included in the wheel/sdist unless configured elsewhere. If you intend to ship the entire tree, prefer a `setuptools.packages.find` configuration (e.g. `[tool.setuptools.packages.find]`) over a single hardcoded package name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| dependencies = [ | ||
| "numpy>=1.26.4", | ||
| "uvloop>=0.21.0", | ||
| "asyncio==3.4.3", |
There was a problem hiding this comment.
issue (bug_risk): Pinning asyncio from PyPI conflicts with the stdlib asyncio on Python ≥3.10.
Given requires-python = ">=3.10.0", asyncio should come from the standard library; the PyPI asyncio package is a legacy backport. Pinning asyncio==3.4.3 risks shadowing the stdlib module and causing subtle runtime issues. Please remove this dependency and rely on the stdlib version, or, if the backport is truly required, document the rationale and ensure it cannot override the built-in module on supported platforms.
|
|
||
| [tool.setuptools] | ||
| license-files = ["LICENSE"] | ||
| packages = ["datamodel"] |
There was a problem hiding this comment.
suggestion (bug_risk): Hardcoding packages = ["datamodel"] may omit nested subpackages that were previously auto-discovered.
Previously find_namespace_packages in setup.py auto-discovered all subpackages under datamodel (e.g. datamodel.rs_parsers, datamodel.libs.*). With packages = ["datamodel"], those subpackages may no longer be included in the wheel/sdist unless configured elsewhere. If you intend to ship the entire tree, prefer a setuptools.packages.find configuration (e.g. [tool.setuptools.packages.find]) over a single hardcoded package name.
Summary by Sourcery
Modernize package build and metadata configuration to rely on pyproject.toml instead of setup.py-driven metadata, and bump the library version.
New Features:
Enhancements:
Build:
Documentation:
Chores: