Skip to content

Modernize Python tooling#36

Closed
0xDEC0DE wants to merge 2 commits into
NicolasLM:masterfrom
0xDEC0DE:issue/33
Closed

Modernize Python tooling#36
0xDEC0DE wants to merge 2 commits into
NicolasLM:masterfrom
0xDEC0DE:issue/33

Conversation

@0xDEC0DE
Copy link
Copy Markdown
Contributor

Move all configs into pyproject.toml, switch test/packaging to use Hatch, and do all the linting with Ruff.

Fix issues that Ruff turned up, and annotate/ignore the pieces that it got wrong.

Fixes: Issue #33

@0xDEC0DE 0xDEC0DE marked this pull request as draft February 28, 2024 19:01
@0xDEC0DE 0xDEC0DE marked this pull request as ready for review February 28, 2024 19:14
Comment thread spinach/brokers/base.py Outdated
Comment thread spinach/worker.py Outdated
Comment thread .github/workflows/python-publish.yml Outdated
Copy link
Copy Markdown
Collaborator

@bigjools bigjools left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed the ticket yesterday so I had something to work on today :(

This change is doing WAY too much all at once; the intention of the ticket was to only replace setup.py with pyproject.toml, and switch the build system to hatchling and setuptools_scm. I specifically did not want to remove tox (yet) nor change linters, the publisher action, or anything like that. (Side note: you have not updated any documentation.)

Comment thread .github/workflows/python-publish.yml Outdated
@0xDEC0DE 0xDEC0DE marked this pull request as draft February 28, 2024 22:36
@0xDEC0DE
Copy link
Copy Markdown
Contributor Author

I filed the ticket yesterday so I had something to work on today :(

This change is doing WAY too much all at once; the intention of the ticket was to only replace setup.py with pyproject.toml, and switch the build system to hatchling and setuptools_scm. I specifically did not want to remove tox (yet) nor change linters, the publisher action, or anything like that. (Side note: you have not updated any documentation.)

It's not really that much change, it just looks like it.

The twine publisher method should still work, and that piece is a separate commit, so it can be dropped and handled later.

Most of what looks like churn is simply copy+paste of discrete config files into the corresponding pyproject.toml section. It really does do it all!

The linter changes are similarly low-effort; my original approach was to drop in Ruff with a giant stack of ignores, and promise to peel them back later. I can do that instead, if it makes things easier.

@0xDEC0DE
Copy link
Copy Markdown
Contributor Author

Downsized for the sake of sanity.

@0xDEC0DE 0xDEC0DE marked this pull request as ready for review February 29, 2024 01:04
@0xDEC0DE 0xDEC0DE force-pushed the issue/33 branch 2 times, most recently from 45aa1b4 to 3e5a727 Compare April 12, 2025 12:55
Copy link
Copy Markdown
Collaborator

@bigjools bigjools left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to have ramifications on the release procedure as it will now react to the tag rather than the version in const.py, I'll check it all out when I get some time unless Mr Le Manchet beats me to it.

@0xDEC0DE 0xDEC0DE requested a review from bigjools April 14, 2025 03:45
Move all configs into `pyproject.toml`, switch test/packaging to
use Hatch, and do all the linting with Ruff.

Fix issues that Ruff turned up, and annotate/ignore the pieces that
it got wrong.

Fixes: Issue NicolasLM#33
Generate the documentation with `hatch run docs:build $TYPE`
Comment thread spinach/__init__.py
@juledwar
Copy link
Copy Markdown
Contributor

I may close this and re-do it using UV exclusively, as it avoids some of the Hatch foibles.

@juledwar
Copy link
Copy Markdown
Contributor

A review from Claude below. It can serve as a basis for the transition to a pure UV project.

Issues

Missing Python 3.13 classifier — The old setup.py and the CI matrix both include 3.13, but the new pyproject.toml classifiers stop at 3.12.

Typo: detatcheddetached — In [tool.hatch.envs.docs], the key is misspelled. Hatch may silently ignore it and install docs dependencies into the default environment instead of a standalone one.

F401 is both ignored globally and per-fileF401 appears in the global ignore list and in per-file-ignores for init.py. The global ignore makes the per-file one redundant. Based on the comment, the intent is to only suppress F401 in init.py files — so it should be removed from the global ignore list.

F841, S301, S311, B024, B027 suppressed globally — I see the TODO about these, but suppressing unused variables (F841) and security-relevant findings (S301 pickle, S311 pseudo-random) project-wide can hide real bugs. Worth fixing the individual occurrences and using noqa comments instead of global suppression, or at least using per-file-ignores to scope them tightly.

Coverage file location — The CI still relies on AndreMiras/coveralls-python-action finding .coverage in the working directory. Confirm that hatch run ci doesn't write it inside .hatch/ where the Coveralls step can't find it.

Minor

  • The contributing docs say "there is a convenience pyproject.toml that runs all the tests" — reads a bit oddly. Maybe "the pyproject.toml defines Hatch scripts that run all tests..."?
  • license-files = { paths = ["LICENSE"] } uses the older table-key format. PEP 639 prefers license-files = ["LICENSE"]. Not blocking, but since this is a modernization PR it might be worth using the latest form.
  • [tool.hatch.envs.default] and [tool.hatch.envs.docs] both set path = ".hatch" — multiple environments sharing the same path is unusual and may cause conflicts.

@0xDEC0DE
Copy link
Copy Markdown
Contributor Author

This has been sitting around for so long, it now needs its own modernisation effort.

Closing.

@0xDEC0DE 0xDEC0DE closed this Apr 23, 2026
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.

3 participants