-
-
Notifications
You must be signed in to change notification settings - Fork 901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Diffable.diff is misleadingly annotated regarding special other
values
#1874
Comments
Thanks a lot for documenting these issues with the Thus I am not surprised that this is notoriously difficult to annotate, but am happy that this seems to be possible.
I'd argue that a failing mypi check won't constitute as breaking change, it's a grey-zone I am OK to walk into if it one day allows type-checkers to make using GitPython easy (due to them actually being accurate). This is one of the few areas where GitPython can still change as well. Knowing that such a change might break somebodies CI, the only constraint I'd apply to myself is that such 'breaking' changes shouldn't be done lightheartedly. |
Speaking of this, in #1859 I should probably have raised the question of what to call the type of the enumeration in
There is also the question of singular and plural. This enumeration is named as a namespace of the constants it defines, rather than in the style one would ordinarily name a type: it is awkward to say " Possibilities like (In contrast, code using GitPython would have trouble annotating its own functions--if it needs to use this type--if the type were nonpublic, which its exclusion from the subpackage If this is to be changed, it would be best changed before the next release, after which the old name may have to be kept as an alias of the new name in order to avoid breakage. (Removing the old name after a release would probably be an actual breaking change because it would not just break type checking but would also cause runtime errors to occur in imports and elsewhere, in reasonable code written after the first name was added and before it was removed.) To be clear, I'm not saying this needs to change, only that there may be a benefit to considering the name and deciding if it does. |
Thanks for highlighting these possible concerns. Since naming is hard, and those names didn't strike me as problematic, I'd think it's good to go with them as the new state of affairs is certainly better than the previous one. |
The
other
parameter ofDiffable.diff
is annotated this way:GitPython/git/diff.py
Line 110 in e880c33
Diffable.diff
accepts a few special values:None
for a working tree diff.Diffable.Index
, atype
object that is meant to be used opaquely rather than instantiated, to compare to the index.NULL_TREE
, a direct instance ofobject
(not to be confused withObject
), for a diff against an empty tree.The type of
NULL_TREE
should be expressed better thanobject
The main problem here, which exists in the documentary effect of the annotations even if a type checker is never used, is the presence of
object
in the union. Sinceobject
is the root of the type hierarchy in Python, a union of anything withobject
is equivalent to justobject
. But the intent of the annotation is not to express that it is correct to pass arbitrary objects asother
. Instead, the idea is that it is okay to passNULL_TREE
.This is also the cause of the incompatible override type error on the
IndexFile.diff
override ingit.index.base
, which does not include theobject
alternative:GitPython/git/index/base.py
Line 1482 in e880c33
There are several places in GitPython's source code where an overridden method has an incompatible signature, violating substitutability. The reason this place is of interest is that it is hard to tell efficiently by examining the code or type errors what conceptually this means. It turns out it means the override does not support
NULL_TREE
:But
object
does not express (to humans or tools) that what is really meant is the literal valueNULL_TREE
.We cannot currently write
Literal[NULL_TREE]
, but ifNULL_TREE
is made into an enumeration constant, then this can be done, though cumbersomely it seems it can only be written--in annotations--asET.NULL_TREE
whereET
is the declared enumeration type. Or, ifET
hasNULL_TREE
as its only value, then writingET
should be sufficient, though this may be less intuitive due to not involvingLiteral
in any way while expressing a literal.To be clear, this will not cause the mypy error about an incompatible override to go away. But it will make the error make sense, as well as making the suppression comment on it make sense in terms of what it is expressing. And it will allow type checkers to catch errors about incorrect values of
other
.Diffable.Index
should ideally also be improvedDiffable.Index
is used as an opaque constant, as isNULL_TREE
. ButDiffable.Index
is a class. This is confusing because it should not be instantiated. Its static type can be expressed pretty specifically, asType[Diffable.Index]
(as is done; I'm omitting the quotes here for simplicity). However, this does not allowif
-else
logic to be type-checked exhaustively, because there is no guarantee that theDiffable.Index
class is the only value of the static typeType[Diffable.Index]
:Hopefully no one is doing that--just as hopefully no one is instantiating it--but
mypy
and other tools, including editors, cannot tell thatDiffable.Index
is the only value ofType[Diffable.Index]
. Humans who notice that it is intended to be used as an opaque constant will recognize this, however. Also, it should be possible to get type checkers to recognize it as the only value, and type-checkif
-else
logic exhaustively, by decorating the definition ofIndex
with@final
. This works for some type checkers, at leastpyright
andpylance
. But it does not work formypy
.The bigger issue is that it is easy to confuse and, for example, instantiate it.
Diffable.Index
can also be made an enumeration constant, and all reasonable usage will continue to work. At runtime. However, if someone wants to statically annotate their own method to accept or return it, then they would need to change the annotations, since expressing it withType[Diffable.Index]
would no longer be able to work. (Two separate special objects cannot both be used, because existing subclasses ofDiffable
outside of GitPython would only be covering one. But if it is redefined, existing code can continue to cover it at runtime, since it should only ever have been, or be, used as an opaque constant.)It is also possible for unreasonable usage to break at runtime, such as attempting to check if
x
isDiffable.Index
by writingissubclass(x, Diffable.Index)
. I am less worried about this.It seems to me that this change is worth making for
Diffable.Index
as well, but this is a judgment call, and the new interface should perhaps be given special attention in review. When I searched for existing code outside GitPython, its forks, vendored copies of it, etc., that used these annotations in a way where they would cause a new static type error, I didn't find anything, but I only used GitHub code search, and I may not have found everything even on GitHub. My argument for making this change is to achieve greater correctness without breaking anything at runtime, and not that I am at all confident nobody will have to change their code to keep it passing static checks.Connection to #1859
I've included fixes for both of these things in #1859. See 65863a2 especially, whose docstring has some more information about the specific design choices there.
If you decide you don't want the change to
Diffable.Index
there, that can be undone without sacrificing the rest. Although I am in favor of this aspect of the change, it should not be accepted on the basis of the sunk-cost fallacy. Furthermore, the sunk cost would be quite low, because it was already helpful in enabling me to figure out what needed to be done in other related parts of the code.The purpose of this issue is twofold:
The text was updated successfully, but these errors were encountered: