Skip to content

Commit 0e1df29

Browse files
committed
Start fixing diff and _process_diff_args type annotations
This fixes three mypy errors by modifying the Diffable.diff, Diffable._process_diff_args, and its IndexFile._process_diff_args override. This change, so far, adds no suppressions, and even removes some preexisting suppressions that were ineffective because of how they were written (not being on the first line of the def). However, this is not a complete fix of those annotations themselves. This is because although the `other` parameter to Diffable.diff and Diffable._process_diff_args does not appear intended to have been as broad as including object in the union had the effect of making it -- any union with object as an alternative is equivalent to object itself -- it should nonetheless be broader than the changes here make it. Once that is fixed, it may not be possible to maintain compliance with the Liskov substitution principle, in which case a suppression, corresponding to one of those that was removed but fixed so it has an effect, may need to be introduced, since actually fixing the LSP violation would be a breaking change. Specifically, as seen in 797e962 and 09053c5 in gitpython-developers#1285, the object alternative was originally intended not to indicate that any object is allowed, but instead to allow the NULL_TREE constant, which was (and currently remains) implemented as a direct instance of object. The type of NULL_TREE is not the core problem. It can be fixed to allow a narrowly scoped annotation. One way is by making NULL_TREE an enumeration constant and annotating `Literal[NULL_TREE]`. Rather, the problem is that the IndexFile.diff override really does not accept NULL_TREE. It may not be feasible to modify it to accept it. Even if that is to be done, it should be for runtime correctness, and likely have a test case added for it, and may not be suitable for inclusion alongside these static typing fixes. So when the base class method's annotation is broadened to add such a literal or equivalent, the override's annotation in the derived class may not reasonably be able to change accordingly, as LSP would dictate. Part of the change here adds a missing os.PathLike[str] alternative to _process_diff_args. For now I've opted to include both str (already present) and os.PathLike[str] separately, rather than consolidating them into PathLike (in GitPython, PathLike is git.types.PathLike, which is Union[str, os.PathLike[str]]). The reason is that, while some str arguments may be paths, others may be options. However, that stylistic choice may not be justified, since it is at odds with some existing uses of PathLike in GitPython to cover both str as a path and str as a non-path, including in the implementation of Diffable.diff itself. So those may be consolidated in a forthcoming commit.
1 parent ed6ead9 commit 0e1df29

File tree

2 files changed

+11
-9
lines changed

2 files changed

+11
-9
lines changed

Diff for: git/diff.py

+7-5
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# This module is part of GitPython and is released under the
44
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
55

6+
import os
67
import re
78

89
from git.cmd import handle_process_output
@@ -98,8 +99,9 @@ class Index:
9899
"""Stand-in indicating you want to diff against the index."""
99100

100101
def _process_diff_args(
101-
self, args: List[Union[str, "Diffable", Type["Diffable.Index"], object]]
102-
) -> List[Union[str, "Diffable", Type["Diffable.Index"], object]]:
102+
self,
103+
args: List[Union[str, os.PathLike[str], "Diffable", Type["Index"]]],
104+
) -> List[Union[str, os.PathLike[str], "Diffable", Type["Index"]]]:
103105
"""
104106
:return:
105107
Possibly altered version of the given args list.
@@ -110,7 +112,7 @@ def _process_diff_args(
110112

111113
def diff(
112114
self,
113-
other: Union[Type["Index"], "Tree", "Commit", None, str, object] = Index,
115+
other: Union[Type["Index"], "Tree", "Commit", str, None] = Index,
114116
paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
115117
create_patch: bool = False,
116118
**kwargs: Any,
@@ -159,7 +161,7 @@ def diff(
159161
:class:`~Diffable.Index`, or as :class:`~git.objects.tree.Tree` or
160162
:class:`~git.objects.commit.Commit`, or a git command error will occur.
161163
"""
162-
args: List[Union[PathLike, Diffable, Type["Diffable.Index"], object]] = []
164+
args: List[Union[PathLike, Diffable, Type["Diffable.Index"]]] = []
163165
args.append("--abbrev=40") # We need full shas.
164166
args.append("--full-index") # Get full index paths, not only filenames.
165167

@@ -195,7 +197,7 @@ def diff(
195197

196198
args.insert(0, self)
197199

198-
# paths is a list here, or None.
200+
# paths is a list or tuple here, or None.
199201
if paths:
200202
args.append("--")
201203
args.extend(paths)

Diff for: git/index/base.py

+4-4
Original file line numberDiff line numberDiff line change
@@ -644,9 +644,9 @@ def write_tree(self) -> Tree:
644644
return root_tree
645645

646646
def _process_diff_args(
647-
self, # type: ignore[override]
648-
args: List[Union[str, "git_diff.Diffable", Type["git_diff.Diffable.Index"]]],
649-
) -> List[Union[str, "git_diff.Diffable", Type["git_diff.Diffable.Index"]]]:
647+
self,
648+
args: List[Union[str, os.PathLike[str], "git_diff.Diffable", Type["git_diff.Diffable.Index"]]],
649+
) -> List[Union[str, os.PathLike[str], "git_diff.Diffable", Type["git_diff.Diffable.Index"]]]:
650650
try:
651651
args.pop(args.index(self))
652652
except IndexError:
@@ -1478,7 +1478,7 @@ def reset(
14781478

14791479
# @ default_index, breaks typing for some reason, copied into function
14801480
def diff(
1481-
self, # type: ignore[override]
1481+
self,
14821482
other: Union[Type["git_diff.Diffable.Index"], "Tree", "Commit", str, None] = git_diff.Diffable.Index,
14831483
paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
14841484
create_patch: bool = False,

0 commit comments

Comments
 (0)