Update to modern packaging #141
Conversation
|
Oops, sorry about the CI failures, I had neglected to update the Github Actions config. Could you give it another go? |
96c968d to
4912651
Compare
2447c58 to
78a6cbb
Compare
|
Alright, this should be green for real now. I worked out the kinks by running CI in my fork. |
Use pyproject.toml, uv, and hatchling. This produces the following layout using uv pip install --no-deps --prefix=/tmp/test-prefix .` /tmp/test-prefix/.lock /tmp/test-prefix/bin/git-revise /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/merge.py /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/odb.py /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/__init__.py /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/utils.py /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/tui.py /tmp/test-prefix/lib/python3.12/site-packages/gitrevise/todo.py /tmp/test-prefix/lib/pyth 8000 on3.12/site-packages/gitrevise/__main__.py /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/INSTALLER /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/REQUESTED /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/licenses/LICENSE /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/direct_url.json /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/RECORD /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/entry_points.txt /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/WHEEL /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/METADATA /tmp/test-prefix/lib/python3.12/site-packages/git_revise-0.7.0.dist-info/uv_cache.json /tmp/test-prefix/share/man/man1/git-revise.1
Batch all updates (per ecosystem) into a single PR once a month.
|
@mystor does this look reasonable to you? |
|
@mystor gentle ping. |
| $ isort . # sort imports | ||
| $ black . # format all python code | ||
| $ uv run isort . # sort imports | ||
| $ uv run ruff format # format all python code |
There was a problem hiding this comment.
I take it here ruff is a new replacement for black? Is the formatting the same as the existing code?
There was a problem hiding this comment.
Yes, ruff format is mostly the same apart form some obscure edge cases. Thy actually have some testing against black formatting.
You can probably also remove isort, that's also taken care of by check --fix-only --select I --fixable I ("I" are the isort rules). If you have any other linters (flake8, pylint), you can probably also replace them with ruff check --select 'ALL'...
There was a problem hiding this comment.
Yeah, It does appear that this patch adds a call to ruff check, though it does not remove the pylint check (which appears to still be called). Is ruff check preferable to pylint nowadays?
There was a problem hiding this comment.
It depends: ruff check (replacement for a lot of flake8 thingies and parts of pylint; an isort) is different from ruff format (replacement for black), so IMO the patch is right to not remove pylint (or at least not remove it without a replacement)
Re comparison with pylint: I would guess it implements about 2/3 of pylints rules (source). Mostly everything which needs context which is outside of the current file is not implmented: ruff is strictly one file only. They have some chatter that multiple files linting is on the radar, but thats seems to be still a bit off as well: "red not", which also will be a mypy replacement, if I understand that right).
My personal take: ruff format + ruff check --select ALL + mypy catches about everything what I care to catch. We have implemented that setup at work (with mypy in struct modusl and a handful of explicitly ignored ruff rules), where it runs against sonarqube and mostly leaves nothing for sonarqube to find. And it's so damn fast that running linter and formatter is fun again :-)
| @@ -31,6 +31,8 @@ | |||
| if TYPE_CHECKING: | |||
| from subprocess import _FILE | |||
|
|
|||
| from typing_extensions import Self | |||
There was a problem hiding this comment.
I don't love needing to pull in a dependency for Self when we can do the existing TypeVar stuff and it looks like it should work OK? What does this do differently?
There was a problem hiding this comment.
Depends how recent you wanna be: it's included in python proper as of py3.11: https://peps.python.org/pep-0673/
from typing import SelfThere was a problem hiding this comment.
Oh that's nice.
Looks like I last increased the minimum python version in 2022 from 3.6 to 3.8 (4a7e852). I imagine we can get away with bumping the minimum version again so long as the major LTS distros (debian stable / ubuntu) are shipping that version of Python or newer.
There was a problem hiding this comment.
debian stable (bookworm) is 3.11
Ubuntu 22.04 is on 3.10 and 24.04 on 3.12
Not sure how dependent you are on system python for installing a newer version of any python package though: if you can install a newer python package you probably also can install (and probably already are installing) a newer python :-)
With uv, you simply do a uv tool install git-revise@latest you probably anyway get a py 3.13^W3.12 installed:
[16:34:18] λ uv tool install git-revise@latest
Resolved 1 package in 282ms
Audited 1 package in 0.00ms
Installed 1 executable: git-revise
[23:04:45] λ which git-revise
~/.local/bin/git-revise
[23:05:32] λ /home/jankatins/.local/share/uv/tools/git-revise/bin/python --version
Python 3.12.8
(That 3.12 is downloaded and installed by uv -> In this case it was already downloaded and extracted/hardlinked)
There was a problem hiding this comment.
Cool. I'll admit I'm out-of-the-loop on these modern python packaging changes, and so I worry about things like "can someone on the Ubuntu LTS do pip3 install --user git-revise and have it work", but I suppose that's less of a thing folks are doing nowadays?
I think I'd definitely be OK with bumping the python version to 3.10, less certain about 3.11 given 22.04 is still on 3.10, but perhaps that is also OK.
| [build-system] | ||
| requires = ["hatchling"] | ||
| build-backend = "hatchling.build" | ||
|
|
There was a problem hiding this comment.
What is hatchling, and how is it connected to uv? On first look it appears it's another different build system?
There was a problem hiding this comment.
uv as of today has no officially released build backend: poetry has poetry-core, hatch has hatchling, pip has .. build? There is also a setuptools one. So essentially you have to pick one and some more.
uv us currently developing one, but that looks still a bit rough. astral-sh/uv#3957 Might already be enough though...
There was a problem hiding this comment.
Arch Linux packager here...
I fully support the switch to a proper PEP 517 build, and the ruff and uv tooling is almost universally recognized as better these days than black and pip and such. I don't have strong feelings about hatch/hatchling over poetry, flit, setuptools, or others, so that's fine I suppose.
On the other hand just a warning that the uv build backend is not only rough but controversial. I would avoid it for the foreseeable future as it introduces lots of binary requirements on the host environment when pure Python would do fine. It's very heavy weight for an application like this.
There was a problem hiding this comment.
That works for me, happy to use whatever works best for folks building the actual packages :-)
| $ twine upload dist/* | ||
| $ uv build | ||
| $ uv run twine check dist/* | ||
| $ uv run twine upload dist/* |
There was a problem hiding this comment.
uv also has some upload capabilities: uv publish... https://docs.astral.sh/uv/guides/package/#publishing-your-package
There was a problem hiding this comment.
That's good to know, might be worth switching to that, but I'd want to look into it more.
| @@ -7,21 +7,24 @@ jobs: | |||
| name: Test on python ${{ matrix.python-version }} and ${{ matrix.os }} | |||
| runs-on: ${{ matrix.os }} | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| python-version: ["3.8", "3.9", "3.10"] | |||
There was a problem hiding this comment.
I guess this could also get an update? https://devguide.python.org/versions/ -> 3.8 is EOL, 3.11-3.13 are missing
There was a problem hiding this comment.
If 3.8 is EOL, I think we can probably drop support, and it'd be nice to start running tests on some more recent python releases, though perhaps that should be a separate PR.
|
Please let me know if changes are required - or go ahead and make what changes you see fit. I've allowed edits by maintainers and you have my explicit consent to edit directly. |
|
Thanks! This seems like a solid direction for modernizing the tooling we're using. |
This is a new feature that appeared after #141.
See individual commits.
I think this likely fixes #140 after a new release.