Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork941
Commit3aeef46
committed
Fix how Diffable annotates expected repo attribute
The Diffable class's essential method, diff, requires self to havea repo attribute, which is outside Diffable's own responsibility toprovide. This was already documented in the Diffable docstring, butthe technique used to prevent mypy from treating accesses to thatattribute as errors had several disadvantages:- Annotating `self.repo` is ambiguous, since typically *names* are annotated. Although mypy treated this afterwards, even in code appearing outside the implementation of Diffable.diff, to mean that Diffable instances have repo attributes, it is not clear how other type checkers, or possibly future version of mypy, would or should interpret this. It is also not obvious from reading the code that it should have an effect on static analysis of code outside the diff method body, but this can be verified by temporarily placing this code (unindented) after, and outside, the Diffable class: from git.types import Has_Repo def f(x: Has_Repo) -> None: pass f(Diffable()) This produces no new type errors from mypy, but if the annotated conditional attribute rebinding statement is commented out, then one of the type errors is that `f(Diffable())` passes an argument of the wrong type to `f`.- The annotation was on an attribute binding operation (assigning to `self.repo`), which mypy accurately reported as an error because the Diffable class had no `repo` instance attribute indicated in the code elsewhere and is a slotted class whose instances have no instance dictionaries to allow that actual binding operation to succeed. In addition to being an incorrect annotation, this had the effect of decreasing the number of related mypy errors only to one, instead of the hoped-for zero.- The rebinding of self.repo to itself was written in way that, if the statement were executed, rebinding would actually occur when the instance has a repo attribute. (See below for why this does not actually happen.) But this depends on the presence of a repo attribute to read from and then write to, so any change that allows mypy to infer that it is okay should also be enough to avoid the original mypy errors at issue in the first place.- If the statement were executed, it would fail at runtime if the instance had a repo attribute that were read-only. But there is no reason a subclass or sibling class used to provide the repo attribute should be unable to do this, such as by defining repo as a property with only a getter.- The condition that guarded the self.repo (re)binding operation was unintentionally incorrect in a way that caused it to be entirely unrelated to the git.typing.Has_Repo protocol it tried to refer to. The check was `hasattr(self, "Has_Repo")`, but the goal is not to check for an attribute named `Has_Repo`, but to check for an attribute named `repo`. With `Has_Repo` imported, the different condition `isinstance(self, Has_Repo)` would do that (the protocol is runtime-checkable). Or the condition `hasattr(self, "repo")` could be used. Instead, it worked the same as under the change: - if hasattr(self, "Has_Repo"): + if hasattr(self, "Any_Other_Nonexistent_Attribute"): The way it avoided a runtime error (and provably so, to mypy) was that it was always False, and mypy simply treated the annotation as usable information on both branches.This commit removes that code and instead adds a class-levelannotation for a `repo` instance attribute, with no assignments.That eliminates the mypy error for the attempt to conditionallyannotate `self.repo`, while also keeping the self.repo accessesfrom becoming static type errors again.As before, mypy treats Diffable as a static subclass of Has_Repo,but if an instance `x` of Diffable lacked a `repo` attribute (whichwould lead to a runtime error, as before, in realistic use, when`diff` were called on it), then `isinstance(x, Has_Repo)` willevaluate to `False`. Because the repo member of Has_Repo is notwritten as a method, it is a runtime error to use issubclass tocheck if a class is a subclass of it (even though isinstance doeswork), so adding this annotation to the Diffable class does notaffect that either.Some behavior of Has_Repo may not be as expected, because of thedifferent semantics of (a) the static type system checked by mypyand other static type checkers and (b) the actual dynamic typesystem of Python enforced at runtime by the implementation.Furthermore, this protocol is not used anywhere in GitPython, andsince this commit, there is no attempt to use or reference it.Because it is public in the public git.types module, it should notbe immediately removed, but it may make sense to deprecate it.However, this commit does not make that change.1 parent4083dd8 commit3aeef46
1 file changed
+5
-5
lines changedLines changed: 5 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
84 | 84 | | |
85 | 85 | | |
86 | 86 | | |
87 | | - | |
88 | | - | |
| 87 | + | |
| 88 | + | |
89 | 89 | | |
90 | 90 | | |
91 | 91 | | |
92 | 92 | | |
93 | 93 | | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
94 | 97 | | |
95 | 98 | | |
96 | 99 | | |
| |||
169 | 172 | | |
170 | 173 | | |
171 | 174 | | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | 175 | | |
176 | 176 | | |
177 | 177 | | |
| |||
0 commit comments
Comments
(0)