lint: switch Black with `ruff-format` #1865

Borda · 2024-03-11T20:35:09Z

following #1862 we could also use just Ruff for better consistency between lint and formatter

git/cmd.py

Byron

Thanks a lot! Finally being compatible to black sounds like a great achievement!

Generally, I have no opinion on how the code looks like, as long as the majority of tools, linters and formatters agree on it, and the majority of people would think that's how it should be.

Putting GitPython in the hands of ruff and black with default settings seems like it lifts a huge burden - these tools are responsible and what they decide is considered desirable.

@EliahKagan could you also approve this PR? I think your comment indicates you are happy with it, but I wanted to be sure before merging.

Thanks in advance, I am looking forward to get this merged.

- `make lint` no longer lints without making changes. This has been the case since gitpython-developers#1862, because `--fix` is used. (I don't think anyone was really using `make lint`, but just in case, we should avoid saying it still never changes any files.) With gitpython-developers#1865, there will be more ways that files can be changed. - `black` is no longer used, having been replaced by `ruff`. This will be the case as of gitpython-developers#1865. This also reorganizes the "Specific tools" list, since they are all configured in pyproject.toml now (only flake8 was not before, and it was removed in favor of ruff in gitpython-developers#1862). In doing so, I've also added brief parenthesized phrases to characterize what each of these four tools is for, so readers don't have to look around as much to understand most of the tooling GitPython has set up.

This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. As noted in c66257e, linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing with gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), is not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It is an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already does not achieve its goal since gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. This commit *avoids* doing any of the following, even though it may be useful to do them later: - This does not give specific instructions in the readme for installing and running ruff (and c66257e before this also omits that). This can be added later and the best way to document it may depend on some other upcoming decisions (see below). - This does not add ruff to the test extra or as any other kind of extra or optional dependency. Although the test extra currently contains some packages not used for running unit tests, such as pre-commit and mypy, adding Ruff will cause installation to take a long time and/or or fail on some platforms like Cygwin where Ruff has to be built from (Rust) source code. This can be solved properly by reorganizing the extras, but that is likely to wait until they are expressed completely inside pyproject.toml rather than one per requirements file (see discussion in comments in gitpython-developers#1716 for general information about possible forthcoming changes in how the project is defined). - This does not update tox.ini to run ruff directly, which could be done by splitting the current lint tox environment into two or three environments for Python linting, Python autoformatting, and the miscellaneous other tasks performed by pre-commit hooks, only the latter of which would use the pre-commit command. Whether and how this should be done may depend on other forthcoming changes. - This does not remove or update the Makefile "lint" target. That target should perhaps not have been added, and was always meant to be improved/replaced, since everything else in the top-level Makefile is for building and publishing releases. See 5d15063 (gitpython-developers#1693). This is likewise not done now since it may depend on as-yet unmerged changes and tooling decisions not yet made. It should be feasible to do together when further updating tox.ini. - This does not update tox.ini, Makefile, or the lint.yml GitHub Actions workflow to omit the manual hook-stage, which will be unused as of gitpython-developers#1865. This would complicate integration of changes, and once it is known that it won't be needed, it can removed. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.

EliahKagan

There is one small change in the Makefile that should be made, commented below, to avoid breaking the lint target (or, also as noted below, the lint target could be removed). But then I think this is ready to go. In particular, the actual style changes, as well the configuration for them, are good. I believe this can be merged immediately after that one fix.

Although there more things that should be updated in the readme and tox configuration, I don't think this needs to wait for them. since they are not specific to this. They instead relate to #1862 just as much as to this. I could've made things a bit simpler if I'd thought of them at that time. Rather than trying to shoehorn them into the PR, I've opened #1868. As noted there, I think it is okay to merge this PR even before #1868 is reviewed.

Makefile

EliahKagan

Looks great!

Byron · 2024-03-13T06:26:34Z

Thanks everyone, great and much appreciated work!

This is to make it so simple `tox` usage has the expected property of leaving all source code files in the working tree unchanged. Linting how sometimes performs auto-fixes since gitpython-developers#1862, and the pre-commit command in tox.ini, which had also run `black --check`, will do even more file editing due to the changes in gitpython-developers#1865. The bifurcation for black into separate mutating and non-mutating hooks, introduced in 5d8ddd9 (gitpython-developers#1693), was not carried over into Ruff autoformatting in gitpython-developers#1865. But also it: - Was not necessarily a good approach, and likely should not be preserved in any form. It was an unusual and unintuitive use of pre-commit. (It can be brought back if no better approach is found, though.) - Was done to avoid a situation where it was nontrivial to set up necessary dependencies for linting in the GitPython virtual environment itself, because flake8 and its various plugins would have to be installed. They were not listed in any existing or newly introduced extra (for example, they were not added to test-requirements.txt) in part in the hope that they would all be replaced by Ruff, which happened in gitpython-developers#1862. - Already did not achieve its goal as of gitpython-developers#1862, since it was (probably rightly) not extended to Ruff linting to use/omit --fix. Now that Ruff is being used, people can run `pip install ruff` in a virtual environment, then run the `ruff` command however they like. This takes the place of multiple tools and plugins. The situation with the tox "lint" environment is thus now similar to that of the tox "html" environment when it was added in e6ec6c8 (gitpython-developers#1667), until it was improved in f094909 (gitpython-developers#1693) to run with proper isolation.

EliahKagan · 2024-03-13T06:43:37Z

I've rebased #1868 to fix the conflict from here and to update and shorten some of its commit messages.

This is no longer used. No pre-commit hook specifies it anymore in `stages`, since 517f83a (gitpython-developers#1865). See b059cd5 (gitpython-developers#1868) for context. In the lint.yml GitHub Actions workflow, this removes the extra_args key altogether, because all that would remain there is --all-files, which is already the default for that action, when the extra_args key is absent.

Although it seems likely that the requirements-dev.txt file will be removed when the project definition is made declarative (discussed in gitpython-developers#1716 comments), if not before, for now it exists and might be in use, so this updates it with tools that are currently used but not listed in any extras or other requirements files: - ruff: This has replaced flake8 and its plugins (gitpython-developers#1862) as well as black (gitpython-developers#1865). Currently there is no separate extra for tooling that is not part of unit testing, with some such tools listed in test-requirements.txt. The `ruff` package belongs here rather than there for now because it should not be installed just to run unit tests, since Ruff has to be built from source on some rarer platforms like Cygwin, which would take a long time and/or fail. - shellcheck: The PyPI package for this is a convenience for installing it in projects that are already using pip (shellcheck is neither written in Python nor a tool to scan Python code). It installs pre-built binaries, which are not available for all platforms. These packages remain listed: - pytest-icdiff: This seems not to have been promoted to be in the test-requirements.txt file and the `test` extra because it does not work as well for diffs from tests that the pytest runner runs but that are written for the unittest framework. - pytest-profiling [commented out]: I am not sure what the status of this is, perhaps it has just not been experimented with enough to know if it would be useful for profiling in GitPython. This requirements-dev.txt file has a few limitations that suggest it should be removed altogether sometime soon: - It is not updated regularly. - It is not always clear why something is there. Originally I believe it was for tools where the desire to use the tool was established but the tool did not yet work or worked but performed checks for which code had to be fixed. That purpose has drifted. - It uses a different naming convention from the test-requirements.txt file in active use. - It cannot be readily used to create an extra in the current project definition in setup.py because the simple parsing done there will not recognize the `-r` lines and will not skip the comments, and neither enhancement should be done in setup.py since that would move things farther away from a declarative project definition. - It will naturally go away when the project definition is made declarative, since it will then be feasible to define as many extras as desired without proliferating separate requirements files (while still allowing their contents to be statically available to tools). Since it may go away soon and is not regularly updated, I have kept the explanations for why particular packages are there out of it. But as long as it exists it may as well list the tools that really are being used yet are not explicitly listed as dependencies.

Borda added 2 commits March 11, 2024 21:33

lint: switch Black with ruff-format

517f83a

apply ruff-format

2d0158c

Borda marked this pull request as ready for review March 11, 2024 20:36

Borda commented Mar 11, 2024

View reviewed changes

git/cmd.py Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

Byron approved these changes Mar 12, 2024

View reviewed changes

EliahKagan mentioned this pull request Mar 12, 2024

Update readme and tox.ini for recent tooling changes #1868

Merged

EliahKagan suggested changes Mar 12, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

drop make lint

1b8812a

Borda requested a review from EliahKagan March 12, 2024 20:10

EliahKagan approved these changes Mar 12, 2024

View reviewed changes

Byron merged commit 3aed2ca into gitpython-developers:main Mar 13, 2024
26 checks passed

Borda deleted the ruff/black branch March 13, 2024 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: switch Black with `ruff-format` #1865

lint: switch Black with `ruff-format` #1865

Borda commented Mar 11, 2024

This comment was marked as resolved.

Byron left a comment

EliahKagan left a comment •

edited

Loading

EliahKagan left a comment

Byron commented Mar 13, 2024

EliahKagan commented Mar 13, 2024

lint: switch Black with ruff-format #1865

lint: switch Black with ruff-format #1865

Conversation

Borda commented Mar 11, 2024

This comment was marked as resolved.

Byron left a comment

Choose a reason for hiding this comment

EliahKagan left a comment • edited Loading

Choose a reason for hiding this comment

EliahKagan left a comment

Choose a reason for hiding this comment

Byron commented Mar 13, 2024

EliahKagan commented Mar 13, 2024

lint: switch Black with `ruff-format` #1865

lint: switch Black with `ruff-format` #1865

EliahKagan left a comment •

edited

Loading