Description
#2037 accidentally introduced 30 new mypy errors, more than doubling the total number that occur on most recent versions of Python from 27 to 57 (or from 28 to 58 in Python 3.13):
diff --git a/a b/b
index 128f3547..59913c9c 100644
--- a/a
+++ b/b
@@ -1,33 +1,82 @@
git\types.py:133: error: Type[...] can't contain "Union[Literal[...], Literal[...]]" [valid-type]
+git\util.py:508: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\util.py:508: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\util.py:512: error: Item Git.AutoInterrupt? of "Union[Popen[Any], Git.AutoInterrupt?]" has no attribute "wait" [union-attr]
git\util.py:1146: error: Variance of TypeVar "T_IterableObj" incompatible with variance in parent type [type-var]
git\util.py:1217: error: Argument 2 to "getattr" has incompatible type "Union[SupportsIndex, str]"; expected "str" [arg-type]
git\util.py:1219: error: Unsupported operand types for + ("str" and "SupportsIndex") [operator]
git\util.py:1219: note: Right operand is of type "Union[SupportsIndex, str]"
git\util.py:1226: error: Unsupported operand types for + ("str" and "SupportsIndex") [operator]
git\util.py:1226: error: Unsupported operand types for + ("str" and "slice[Any, Any, Any]") [operator]
git\util.py:1226: note: Right operand is of type "Union[SupportsIndex, slice[Any, Any, Any], str]"
git\config.py:572: error: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes [str-bytes-safe]
git\config.py:628: error: Redundant cast to "Union[str, PathLike[str]]" [redundant-cast]
+git\cmd.py:98: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:98: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:106: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:106: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:170: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:170: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:218: error: Unused "type: ignore" comment [unused-ignore]
+git\cmd.py:510: error: Variable "git.cmd.Git.CatFileContentStream" is not valid as a type [valid-type]
+git\cmd.py:510: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1051: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1051: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1108: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1108: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1528: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1528: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1533: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1533: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1537: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1537: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1658: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1658: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1668: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1668: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1671: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\cmd.py:1671: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1672: error: Git.AutoInterrupt? has no attribute "stdin" [attr-defined]
+git\cmd.py:1672: error: Git.AutoInterrupt? has no attribute "stdout" [attr-defined]
+git\cmd.py:1673: error: Git.AutoInterrupt? has no attribute "stdin" [attr-defined]
+git\cmd.py:1674: error: Git.AutoInterrupt? has no attribute "stdin" [attr-defined]
+git\cmd.py:1675: error: Git.AutoInterrupt? has no attribute "stdout" [attr-defined]
+git\cmd.py:1703: error: Git.CatFileContentStream? has no attribute "read" [attr-defined]
+git\cmd.py:1707: error: Variable "git.cmd.Git.CatFileContentStream" is not valid as a type [valid-type]
+git\cmd.py:1707: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\cmd.py:1719: error: Git.AutoInterrupt? has no attribute "stdout" [attr-defined]
git\refs\log.py:148: error: Argument 1 to "RefLogEntry" has incompatible type "Tuple[str, str, Actor, Tuple[int, int], Optional[str]]"; expected "Iterable[Union[str, Actor, Tuple[int, int]]]" [arg-type]
+git\diff.py:584: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\diff.py:584: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
git\objects\submodule\base.py:834: error: "SymbolicReference" has no attribute "set_tracking_branch" [attr-defined]
git\objects\submodule\base.py:862: error: "SymbolicReference" has no attribute "tracking_branch" [attr-defined]
+git\objects\commit.py:454: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\objects\commit.py:454: note: Error code "valid-type" not covered by "type: ignore" comment
+git\objects\commit.py:454: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\objects\commit.py:459: error: Git.AutoInterrupt? has no attribute "communicate" [attr-defined]
git\objects\commit.py:711: error: Argument 2 to "create" of "SymbolicReference" has incompatible type "Callable[[], SymbolicReference]"; expected "Union[str, PathLike[str]]" [arg-type]
git\objects\commit.py:903: error: Argument 2 to "findall" has incompatible type "Union[str, bytes]"; expected "str" [arg-type]
git\refs\symbolic.py:919: error: Incompatible types in assignment (expression has type "SymbolicReference", variable has type "T_References") [assignment]
git\refs\tag.py:48: error: Read-only property cannot override read-write property [misc]
git\refs\tag.py:49: error: Unused "type: ignore" comment [unused-ignore]
git\refs\tag.py:83: error: Read-only property cannot override read-write property [misc]
git\refs\tag.py:84: error: Unused "type: ignore" comment [unused-ignore]
git\refs\head.py:48: error: Incompatible override of a setter type [override]
git\refs\head.py:48: note: (base class "SymbolicReference" defined the type as "Union[Commit, SymbolicReference, str]",
git\refs\head.py:48: note: override has type "Commit")
git\refs\head.py:48: note: Setter types should behave contravariantly
+git\remote.py:874: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\remote.py:874: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\remote.py:902: error: Git.AutoInterrupt? has no attribute "wait" [attr-defined]
+git\remote.py:945: error: Variable "git.cmd.Git.AutoInterrupt" is not valid as a type [valid-type]
+git\remote.py:945: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
+git\remote.py:975: error: Git.AutoInterrupt? has no attribute "wait" [attr-defined]
git\repo\fun.py:289: error: "Callable[[], SymbolicReference]" has no attribute "commit" [attr-defined]
git\repo\fun.py:339: error: "Callable[[], SymbolicReference]" has no attribute "log_entry" [attr-defined]
git\repo\base.py:688: error: Redundant cast to "Literal['system', 'global', 'user', 'repository']" [redundant-cast]
git\repo\base.py:690: error: Redundant cast to "Literal['system', 'global', 'user', 'repository']" [redundant-cast]
git\repo\base.py:1054: error: Incompatible return value type (got "SymbolicReference", expected "Head") [return-value]
git\repo\base.py:1487: error: Argument 5 to "_clone" of "Repo" has incompatible type "Optional[Optional[Callable[[int, Union[str, float], Union[str, float, None], str], None]]]"; expected "Union[RemoteProgress, UpdateProgress, Callable[..., RemoteProgress], None]" [arg-type]
git\repo\base.py:1548: error: Argument 5 to "_clone" of "Repo" has incompatible type "Optional[Callable[[int, Union[str, float], Union[str, float, None], str], None]]"; expected "Union[RemoteProgress, UpdateProgress, Callable[..., RemoteProgress], None]" [arg-type]
test\deprecation\test_basic.py:137: error: Argument "change_type" to "iter_change_type" of "DiffIndex" has incompatible type "str"; expected "Literal['A', 'D', 'C', 'M', 'R', 'T', 'U']" [arg-type]
-Found 27 errors in 12 files (checked 44 source files)
+Found 57 errors in 15 files (checked 44 source files)The problem is that, as described in Variables vs type aliases, mypy only recognizes assignments to create type aliases when they are a module level, when TypeAlias is used, or when the Python 3.12+ type syntax is used. This is at class level, TypeAlias is not used, and type is not used (and can't be used, because we support versions of Python below 3.12).
So even though Sphinx would readily recognize these as type aliases (hence the need to change __name__ and __qualname__) when we didn't want it to do so, mypy would not recognize them as type aliases even though we do want to either to do so (or to recognize them as nested classes, but I don't think that's feasible to have a static type-checker do, and it may not be desirable).
This will actually affect users unless it is fixed before the next release is made, because Git.AutoInterrupt and Git.CatFileContentStream are public types that users are supposed to be able to access directly.
I'm aware of several possible solutions, some better than others:
-
Revert #2037. This is the simplest solution, but we forgo its organizational benefits. On the other hand, this problem might be viewed as an argument that the tradeoffs aren't worth it.
-
Use
TypeAlias, in an annotation, e.g.AutoInterrupt: TypeAlias = _AutoInterrupt. This is the obvious solution, and it seems to work fine. But the problem is thatTypeAliasis only available in the standard librarytypingmodule since Python 3.10. Before that, it can be obtained from thetyping_extensionspackage. But that package is currently only a dependency ofGitPythonwhen using Python 3.7.I wouldn't hesitate even slightly to require
typing_extensionsfor Python 3.8, which like 3.7 is end-of-life (no longer supported by the Python Software Foundation). People should avoid using EoL versions of Python, and should expect a somewhat degraded experience when doing so. But one of the long-standing benefits of GitPython is its minimal dependencies, so I'm reluctant to impose new ones on users of non-EoL versions without good reason.However,
typing_extensionsis ubiquitous, so I wouldn't expect this to lead to problems for users/developers, nor for downstream packagers (operating systems that packageGitPythonsurely packagetyping_extensionstoo). I think this is the way to go unless further complexities arise when doing it. -
Actually make
AutoInterruptandCatFileContentStreamtop-level classes, and refer to them directly as such. For consistency with the usual expectations for conceptually public top-level classes of modules GitPython, we would probably also expose them in the top-levelgitmodule, by placingimportstatements and__all__entires for them in__init__.py; but whether or not we did so, this is a change that would be non-breaking to implement but breaking to revert, if reverted after a release is made.Abstractly speaking, the main problem here is that, unless it's combined with something like (2), it is an incomplete solution: the
Gitclass will still need to contain aliases, and these will conceptually be aliases. In a sense, this even causes them to conceptually be aliases, in that their relationship to another type that is part of the public interface would be more than an implementation detail. So they should be treated as aliases, not variables, by the type system.Concretely speaking, the main problem here is that code outside GitPython would still get the
mypyerrors, it would just have a way to make them go away. That's sometimes acceptable or even good--the point of static type checking is to find errors--but in this case the errors don't correspond to actual problems in the client code where they would arise. Therefore, I think that while it may be that this should be done eventually, it shouldn't be done as part of solving this bug.(Also, do we intend that users/developers be able to customize the behavior of these classes by making a class that derives from
Gitthat introduces new classes calledAutoInterruptandCatFileContentStreamthat inherit fromGit.AutoInterruptandGit.CatFileContentStream? If so, then we would have to be careful about where we use the module-level classes and where we use theGitmember aliases, even though we intend them to be interchangeable.) -
Change the top-level nonpublic
_AutoInterruptand_CatFileContentStreamto_AutoInterruptBaseand_CatFileContentStreamBase, defineAutoInterruptandCatFileContentStreamas nested classes that inherit from them (and introduce derived__slots__ = ()to avoid introducing__dict__s), and move the docstrings into the derived actually-nested classes.This seems like it would be more confusing to work with when maintaining GitPython, and I am not confident that it will automatically preserve the intended effect with Sphinx, though it probably would. This is also arguably not a situation where the use of inheritance is warranted. But I can definitely look into this further if you think it might be preferable to (2).
-
Hacky ways to get
mypyto recognize the classes as type aliases. For example, maybe placing them in another module and writingimportstatements for them inside theGitclass would work! I think the badness of such approaches is self-evident, but in this case doing something like that would also have no clear benefit over (1), since it would place the classes' substantive definitions in an inconvenient and unintuitive place that would decrease readability.
If (2) is done, we can still revert both it and #2037 to get (1). So I'll do (2) unless further problems arise with it, with the idea that adding a typing_extensions dependency for more versions of Python, even one non-EoL version, may be acceptable.
Edit: This is done in #2039.