You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 #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.
0 commit comments