switch to Ruff instead of `flake8` about gitpython HOT 6 CLOSED

Does it mean that PR is welcome? =)

from gitpython.

Let's wait for @EliahKagan to chime in on this, he has been doing a lot of work recently and has a plan. I say Rust tools are great, but I am biased ;).

from gitpython.

Thanks!

CC @EliahKagan , who has been contemplating with the idea as well.

from gitpython.

Yes, I think this is a great time to switch to Ruff.

The main problem I encountered last time I worked on this is that Ruff didn't understand the special semantics of typing.Literal when it was imported indirectly from GitPython's git.types module. This caused a valuable check not to be usable, due to too many false positives. (Unfortunately I don't remember the details about the check itself. I can look this up and will try to do so, but I wanted to reply now rather than delaying further.)

Literal is different from most types that define __class_getitem__. This is because, for most classes X that support subscripts, X["Y"] means something like X[Y], allowing Y to be referred even if declared afterwards or in an import guarded by if TYPE_CHECKING:. In contrast, Literal["Y"] does not use the type Y as the value of a generic type argument, but instead is a type whose one and only value is the literal string "Y".

Special rules are required to distinguish between them. Static type checkers such as mypy and pyright recognize GitPython's usage where Literal is conditionally imported in git.types either from typing or typing_extensions depending on the Python version. At least as of when I last tried this with Ruff, it did not recognize this. So it thought that subexpressions like "Y" in type annotations like Literal["Y"] referred to a type like "Y" rather than a value, and complained that no such type was defined.

My recollection is that this particular check is one that seems quite valuable for GitPython to have, and that flake8 is currently managing to provide it, without the false positives Ruff gives (or at least, at that time, gave). I am reluctant to disable it. Suppression comments could maybe be used, if they can be made narrow enough. I vaguely (and perhaps wrongly) recall that this seemed unwieldy to do at the time.

An obvious possible solution is for GitPython to drop support for Python 3.7, since typing has Literal since 3.8. After all, Python 3.7 has been end-of-life for quite some time now. Then the imports could be changed to take Literal directly from typing. Although I think GitPython should drop 3.7 support fairly soon--and maybe even immediately if a significant benefit from a 3.8+ feature arises for GitPython's code or its unit tests--it seems to me that tooling changes are not a strong reason to drop 3.7 right now, especially since Ruff still officially supports Python 3.7 even in its latest version.

I am, however, biased in this area, because a feature branch I'm working on (that is related to symlinks on Windows and does not yet have a PR, and which I hope to pick back up after #1859) benefits from the continued inclusion of 3.7, in that 3.7 and later versions of Python differ in their handling of symlinks in a way that reveals weaknesses in the approach I am taking that would still be weaknesses even in the absence of a need to support 3.7. This is to say that, ironically, it benefits from 3.7 because it is hard to support 3.7. I acknowledge that "We should keep 3.7 support for a while longer because it is causing problems" is not a very strong argument. 😸

In spite of this, the reason I say now is a good time to switch to Ruff is that a long-standing flake8-related limitation has come up, relating to its interaction with black. The version of black that will automatically be installed with pip install -e '.[test]' is now newer than the version specified in .pre_commit_config,.yml and used by pre-commit. The new version of black will format type stubs with the ... body on the same line as the : that precedes it (when there is enough space), rather than on a separate line as most function bodies are written. This is the more widely accepted style and it makes sense that black now prefers it. However, this conflicts with flake8's rules. The relevant rule is actually disabled by default, but some projects, including GitPython, enable it (I don't remember if GitPython does this explicitly or through a use of exclude rather than extend-exclude). GitPython doesn't carry any .pyi files, but @overload annotated stubs that precede an actual implementation qualify as type stubs.

Unlike the rule at stake with Literal, this is a fairly unimportant rule, since probably no style checker is needed to avoid writing non-stub functions on one line, and the bad impact of doing so by accident would also be pretty small. So that rule could just be turned off, and the versions for flake8 and its plugins in .pre-commit-config.yml could be updated. And maybe Ruff will even have this problem, too. Yet I still think that makes this a good time to try again in switching to Ruff; the linting setup has to be revisited now-ish anyway, after all.

Because Ruff at least previously had an issue with GitPython's use of Literal from git.types, and #1859 makes heavy changes to the git.types module, as well as changes to numerous type annotations some of which use Literal, it might be easier to wait until that PR is done. But I don't want to keep you waiting in case you want to move faster. (Likewise, I'll try to look up the mentioned rules and, where applicable, relevant GitHub issues, but I didn't want that research to be a blocker here.)

from gitpython.

An obvious possible solution is for GitPython to drop support for Python 3.7, since typing has Literal since 3.8.

in different projects, we used to use from typing_extensions import Literal

from gitpython.

Yes, that may be the way to go.

Depending how it is done, it could require adding typing_extensions as a dependency of GitPython even for Python 3.8 and later. Currently that dependency is for python_version<"3.8". It seems to me that adding it for all versions is not currently justified for this (though it may be justified in the future for forthcoming features, such as PEP 702).

However, when a type checker is installed and used, I believe typing_extensions is available. So if the typing_extensions imports are guarded by if TYPE_CHECKING: then I think it should work even without changing GitPython's main dependencies. (It is fine if typing_extensions is a direct or indirect dependency of an extra; my concern is about the main dependencies.)

This may be a little cumbersome for Literal with string literal subscripts if the whole thing is quoted, but most of those in GitPython are defined as type aliases and the alias name used. (from __future__ import annotations will eliminate the need for the outer quotes, of course. Some GitPython modules use this and most don't; I'm not sure if there's any guideline in GitPython when it comes to that.)

from gitpython.