Diffable.diff is misleadingly annotated regarding special `other` values about gitpython HOT 3 CLOSED

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.

from gitpython.

Thanks a lot for documenting these issues with the diff API. Creating an API like this was clearly a mistake as it's not helping writing more readable code at all due to the overloadedness of the input arguments, all that while 'leveraging' the class hierarchy.

Thus I am not surprised that this is notoriously difficult to annotate, but am happy that this seems to be possible.

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.

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.

from gitpython.

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 git.diff in which NULL_TREE and INDEX (the latter being what Diffable.Index now aliases) are defined. I called it DiffConstants. I mentioned one aspect of this in 65863a2:

the name DiffConstants (rather than, e.g., Constants) should avoid confusion even if it ends up in another scope unexpectedly.

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 "NULL_TREE is a DiffConstants". Since enumerations always define constants, typically they shouldn't have Constants or Constant in their type names, but here it seemed to help distinguish it from everything else in git.diff. In some languages there are strong conventions for naming enumerations that help resolve this sort of thing; for example, in C#, enumerations should have plural names when they provide flags (and have the [Flags] attribute) and should have singular names otherwise. I don't know of such conventions for Python.

Possibilities like DiffSpecial did not seem better. Anything without Diff in its name would be confusing when accessed directly in git. The top-level git module currently uses wildcard imports for almost everything in GitPython, but that can and should change even separately from this, so one approach could be to change that, at least for its imports from git.diff, so as not to list that enumeration's type: NULL_TREE and INDEX should still be imported in git.__init__ and listed in git.__all__, because NULL_TREE was there before, but DiffConstants could defensibly not be imported in git.__init__.

(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 git.diff.__all__ would signify unless otherwise documented, and would appear to signify even if otherwise documented.)

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.

from gitpython.