Modernise build tooling and configuration#120
Conversation
dabapps-robot
left a comment
There was a problem hiding this comment.
Summary / high-level
Nice modernisation overall: moving from setup.py to pyproject.toml and switching the release workflow to python -m build + trusted publishing via pypa/gh-action-pypi-publish are both solid steps forward. The GitHub Actions updates (newer actions, explicit permissions, OIDC) look sensible.
There are a couple of packaging/backwards-compat details worth tightening up before merge to avoid an accidental breaking release.
Specific points
1) django_readers/__init__.py removal (namespace + backwards-compat)
Removing django_readers/__init__.py changes django_readers into a namespace package. That can be fine, but it’s a behavioural change and also interacts with packaging/discovery.
Also, you’ve removed django_readers.__version__, which some downstream users may be importing.
Suggested low-impact approach: keep an __init__.py and (if you still want runtime access to the version) populate __version__ from package metadata so you don’t have to duplicate it:
from importlib.metadata import PackageNotFoundError, version
try:
__version__ = version("django_readers")
except PackageNotFoundError:
__version__ = "0.0.0"(Or omit the fallback if you’d rather force installed-metadata only.)
2) Potentially unintended support-policy change (requires-python)
setup.py had python_requires=">=3.6", but pyproject.toml now has requires-python = ">=3.10", and the publish workflow builds on Python 3.12.
If dropping 3.6–3.9 is intentional, all good—but it’s a breaking change worth calling out in release notes. If it’s not intentional, we should adjust requires-python (and possibly the workflow Python version) to match the intended support range.
3) license metadata format in pyproject.toml
In PEP 621, license is typically a table ({file=...} or {text=...}) rather than a bare string. Some tooling is permissive, but for maximum compatibility I’d suggest switching to e.g.:
license = { file = "LICENSE" }…and optionally keep the SPDX info elsewhere (classifiers / etc.) if you want it surfaced that way.
4) Package data inclusion
The old setup.py explicitly included non-package files under django_readers via get_package_data(). With the new config using [tool.setuptools.packages.find], it’s worth double-checking whether you need to include any package data (if the project ships anything besides .py files).
If you do have package data, we may need include-package-data = true and/or explicit configuration for package data patterns.
Merge readiness
Close, but I’d like to see the decisions around (a) removing __init__.py / __version__, and (b) the Python minimum version bump, clarified and/or adjusted first. Once those are settled, I think this is good to merge.
This attempts to transition the project from the old-school
setup.pystyle to the more modernpyproject.tomlapproach.