-
-
Notifications
You must be signed in to change notification settings - Fork 906
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
Improve static typing and docstrings related to git object types #1859
Commits on Mar 3, 2024
-
This revises the docstring of git.types.assert_never to more clearly express its semantics and usage, to use the type names used in the typing module, typing_extensions package, and mypy. This expands some parts when doing so seemed to benefit clarity. The docstring had previously said AssertionError was raised, but the code raised ValueError. For now I've adjusted the docstring to be accurate to the code's behavior, but maybe this should change. I also removed the part about the mypy error being on variable creation, since I am not clear on what that was referring to, and if it means the first name binding operation for a variable in its scope, then I am not sure why that would be where an error message would be expected when using assert_never. Finally, this slightly adjusts the message: - "Literal" is decapitalized, to decrease confusion if the default message is used when matching on something not a typing.Literal. - The exception message prints the unexpected value's repr rather than its str, since the repr, when different from the str, is usually the representation more useful for debugging.
Configuration menu - View commit details
-
Copy full SHA for f83b056 - Browse repository at this point
Copy the full SHA f83b056View commit details -
Fix unnecessarily long reference in Tree docstrings
There is no advantage to fully qualifying names in the same module as the entity being documented, but I had accidentally done that.
Configuration menu - View commit details
-
Copy full SHA for 01cc8e2 - Browse repository at this point
Copy the full SHA 01cc8e2View commit details -
Change how tree[subscript] is introduced
This modifies the Tree docstring to replace the phrase "Tree as a list" with a phrase that: - Encompasses the use where subscripts are not integers or slices. - Is more precise in its terminology. The former advantage--not leaving out the case where one can pass a string--is the rationale.
Configuration menu - View commit details
-
Copy full SHA for 6f3a20f - Browse repository at this point
Copy the full SHA 6f3a20fView commit details -
Refine how tree[subscript] is introduced
This drops the newly introduced precision in Python terminology, likening usage to that of a list or dict (experienced Python users will already know about sequences and mappings, and inexperienced users will, with these terms, be able to understand what is said). Note that this does not undo the broader effect of the previous commit, which is to make clear that subscripting a tree supports both list-like and dict-like usage, not just list-like usage.
Configuration menu - View commit details
-
Copy full SHA for 85889cd - Browse repository at this point
Copy the full SHA 85889cdView commit details -
Configuration menu - View commit details
-
Copy full SHA for 9e47083 - Browse repository at this point
Copy the full SHA 9e47083View commit details -
Document Tree_ish, Commit_ish, and related types
The Commit_ish docstring may require substantive revision. The follow claims about the reasion for the design of Commit_ish should be checked, and if false removed or fixed, and if true then possibly clarified or otherwise refined: - "often usable where a commit-ish is expected" (both as for whether it is a reasonable generalization and for whether it is actually part of the reason Commit_ish includes Blob and Tree) - "It is done for practical reasons including backward compatibility." (for whether that is why it was done is or kept)
Configuration menu - View commit details
-
Copy full SHA for 3bd8177 - Browse repository at this point
Copy the full SHA 3bd8177View commit details
Commits on Mar 4, 2024
-
Expand docs of classes representing Git objects
This expands the docstrings of the Object base class, as well as the four leaf subclasses that represent git objects (Blob, Commit, TagObject, and Tree) to clarify those classes' relationship to git objects in general and specific types of git objects, to each other, and where applicable to types defined in git.types. This includes links to specific sections of the gitglossary(7) manpage (as hosted online), where directly applicable.
Configuration menu - View commit details
-
Copy full SHA for f3b9a69 - Browse repository at this point
Copy the full SHA f3b9a69View commit details -
Do a bit of tidying related to unused names
In git.types. - Remove commented-out import of SymbolicReference, which is not used anywhere, nor mentioned in commented-out code. - Add a docstring to _T to note that it is used within GitPython. (Otherwise it looks left over, as no code in git.types uses it.)
Configuration menu - View commit details
-
Copy full SHA for 2af7640 - Browse repository at this point
Copy the full SHA 2af7640View commit details -
Configuration menu - View commit details
-
Copy full SHA for 2aa053e - Browse repository at this point
Copy the full SHA 2aa053eView commit details -
Configuration menu - View commit details
-
Copy full SHA for 15d50de - Browse repository at this point
Copy the full SHA 15d50deView commit details -
Fix possible inaccuracy in Lit_config_levels docstring
These are not necessarily what one means by the "scope" of a configuration variable, in part because of the nature of the subtle distinction between "user" and "global", and in part because of other issues such as how setting a variable for a single command with "-c" has a scope which is not listed. This also brings the docstring somewhat more in line with how these values are documented elsewhere in GitPython.
Configuration menu - View commit details
-
Copy full SHA for 7166703 - Browse repository at this point
Copy the full SHA 7166703View commit details -
Use phrases like "git object type" where applicable
In some recently introduced or expanded docstrings, I had overused phrases like "kind of git object" with the hope of avoiding confusion with the meanings of "type" relevant to Python (i.e., "class" or "static type"). But this made the relationship to git's own notion of "object type" less clear than it could be, especially in docstrings that also included links to the gitglossary(7) entry for "object type" (because those links' relevance was less clear). This dials back the use of "kind" to where I am more confident that it is clarifying or at least not confusing.
Configuration menu - View commit details
-
Copy full SHA for 1530fd2 - Browse repository at this point
Copy the full SHA 1530fd2View commit details -
Configuration menu - View commit details
-
Copy full SHA for 2e02b09 - Browse repository at this point
Copy the full SHA 2e02b09View commit details -
Move our PathLike below even TYPE_CHECKING imports
The benefits are that putting imports before newly introduced names (other than names like __all__) is recommended by PEP-8, and that git.types.PathLike is not equivalent to os.PathLike and introducing it after all imports may help avoid obscuring this.
Configuration menu - View commit details
-
Copy full SHA for 012d710 - Browse repository at this point
Copy the full SHA 012d710View commit details -
Remove commented-out is_config_level function
And the commented-out imports that had been solely to support it.
Configuration menu - View commit details
-
Copy full SHA for a06f1fc - Browse repository at this point
Copy the full SHA a06f1fcView commit details -
To make clear that code outside GitPython would not typically benefit from using anything in that module. See gitpython-developers#1854 for context.
Configuration menu - View commit details
-
Copy full SHA for c93e431 - Browse repository at this point
Copy the full SHA c93e431View commit details -
Configuration menu - View commit details
-
Copy full SHA for 29443ce - Browse repository at this point
Copy the full SHA 29443ceView commit details -
Don't bind unused _assertion_msg_format
The _assertion_msg_format module attribute (global variable) of git.objects.base was formerly used in an assertion check that has since been commented out but not completely removed. It may be that both it and the commented-out code that uses it should simply be removed (they will be in the git history, after all), but this change just brings them in line by also commenting out the variable.
Configuration menu - View commit details
-
Copy full SHA for b6e3ad2 - Browse repository at this point
Copy the full SHA b6e3ad2View commit details
Commits on Mar 6, 2024
-
Configuration menu - View commit details
-
Copy full SHA for b5d9198 - Browse repository at this point
Copy the full SHA b5d9198View commit details
Commits on Mar 7, 2024
-
Configuration menu - View commit details
-
Copy full SHA for 2212ac9 - Browse repository at this point
Copy the full SHA 2212ac9View commit details -
Simplify _safer_popen_windows "if shell" logic
This fixes a static typing error reported by mypy. Using a separate variable, as I had done originally, does not seem to be clearer than rebinding the parameter, and in this case the code is simpler and can be checked by mypy without needing another explicit type annotation to be added. This fixes one mypy error. This also adds a comment to make clearer why the mapping passed in is copied when modifications are needed, rather than mutated.
Configuration menu - View commit details
-
Copy full SHA for 3c5ca52 - Browse repository at this point
Copy the full SHA 3c5ca52View commit details -
Annotate safer_popen broad enough for all platforms
This fixes another static typing error reported by mypy. (The annotation could be made more specific in the future by making a custom protocol for it, which may or may not be worthwhile, given that `**kwargs: Any` would still have to be present after whatever typed keyword arguments the protocol's `__call__` method listed, since some callers intentionally forward arbitrary extra keyword arguments through safer_popen to Popen.)
Configuration menu - View commit details
-
Copy full SHA for 43b7f8a - Browse repository at this point
Copy the full SHA 43b7f8aView commit details -
Fix mypy error with creationflags in subprocess module
On Windows, Python's subprocess module contains constants useful to pass to the `creationflags` parameter of subprocess.Popen. These are absent on other platforms, where they are not meaningful. The code was already safe at runtime from any AttributeError related to these constants, because they were only used in git.cmd._safer_popen_windows, which was defined on Windows. But in gitpython-developers#1792 I did not write the code in a way where mypy can verify its correctness. So if a regression of the kind mypy can in principle catch were to occur, it would be harder to notice it. This refactors the code, keeping the same behavior but expressing it in a way mypy can understand. This consists of two changes: 1. Only define _safer_popen_windows when the platform is Windows, placing it in the first branch of the `if` statement. This is needed because mypy will not take the only current call to that nonpublic function being on Windows as sufficient evidence that the platform is always Windows when it is run. 2. Determine the platform, for this purpose, using sys.platform instead of os.name. These are far from equivalent in general (see the deprecation rationale for is_<platform> in gitpython-developers#1732, revised in a0fa2bd in gitpython-developers#1787). However, in Python 3 (GitPython no longer supports Python 2), in the specific case of Windows, we have a choice of which to use, as both `sys.platform == "win32"` and `os.name == "nt"`. os.name is "nt" on native Windows, and "posix" on Cygwin. sys.platform is "win32" on native Windows (including 64-bit systems with 64-bit Python builds), and "cygwin" on Cygwin. See: https://docs.python.org/3/library/sys.html#sys.platform This is needed because the type stubs for the subprocess module use this sys.platform check (rather than an os.name check) to determine if the platform is Windows for the purpose of deciding which constants to say the subprocess module defines. I have verified that neither of these changes is enough by itself.
Configuration menu - View commit details
-
Copy full SHA for dc95a76 - Browse repository at this point
Copy the full SHA dc95a76View commit details -
Refactor kill_after_timeout logic so mypy can check it
This changes how Git.execute defines the kill_process callback and how it performs checks, fixing two mypy errors on Windows about how the signal module doesn't have SIGKILL. In doing so, it also eliminates the need for the assertion added for safety and clarity in 2f017ac (gitpython-developers#1761), since now kill_process is only defined if it is to be used (which is also guarded by a platform check, needed by mypy). As in dc95a76 before this, part of the change here is to replace some os.named-based checks with sys.platform-based checks, which is safe because, when one is specifically checking only for the distinction between native Windows and all other systems, one can use either approach. (See dc95a76 for more details on that.)
Configuration menu - View commit details
-
Copy full SHA for 4191f7d - Browse repository at this point
Copy the full SHA 4191f7dView commit details -
Factor communicate and watchdog logic to helper
The goal is to improve readability by not repeating the platform and kill_after_timeout check three times.
Configuration menu - View commit details
-
Copy full SHA for 1ef3365 - Browse repository at this point
Copy the full SHA 1ef3365View commit details -
Fix new mypy confusion about kill_after_timeout type
The refactoring in 1ef3365 before this added a new mypy error on non-Windows platforms, where mypy failed to infer that the type of kill_after_timeout was `float` (rather than `float | None`) at the point in the code where it was used as a captured variable in the newly introduced communicate() helper. This was even though the variable is never rebound there or in the enclosing scope that introduced it. So introducing and using a new variable that holds the same reference, which is sufficient to fix the problem and is the approach taken here, is not a behavioral change.
Configuration menu - View commit details
-
Copy full SHA for 4083dd8 - Browse repository at this point
Copy the full SHA 4083dd8View commit details -
Fix how Diffable annotates expected repo attribute
The Diffable class's essential method, diff, requires self to have a repo attribute, which is outside Diffable's own responsibility to provide. This was already documented in the Diffable docstring, but the technique used to prevent mypy from treating accesses to that attribute 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-level annotation for a `repo` instance attribute, with no assignments. That eliminates the mypy error for the attempt to conditionally annotate `self.repo`, while also keeping the self.repo accesses from 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 (which would lead to a runtime error, as before, in realistic use, when `diff` were called on it), then `isinstance(x, Has_Repo)` will evaluate to `False`. Because the repo member of Has_Repo is not written as a method, it is a runtime error to use issubclass to check if a class is a subclass of it (even though isinstance does work), so adding this annotation to the Diffable class does not affect that either. Some behavior of Has_Repo may not be as expected, because of the different semantics of (a) the static type system checked by mypy and other static type checkers and (b) the actual dynamic type system of Python enforced at runtime by the implementation. Furthermore, this protocol is not used anywhere in GitPython, and since this commit, there is no attempt to use or reference it. Because it is public in the public git.types module, it should not be immediately removed, but it may make sense to deprecate it. However, this commit does not make that change.
Configuration menu - View commit details
-
Copy full SHA for 3aeef46 - Browse repository at this point
Copy the full SHA 3aeef46View commit details
Commits on Mar 8, 2024
-
Fix how HEAD annotates inherited commit property
HEAD inherits its commit attribute from SymbolicReference, which defines it as a property using an explicit function call to `property`, rather than using it as a decorator. Due at least to python/mypy#16827, mypy has trouble with this. Currently that assignment in SymbolicReference is marked as type[ignore], which suppresses a type error (or something mypy regards as one) in the call itself. Even without that type comment, the type of the SymboilicReference instance attribute produced by the property is inferred as Any. This commit does not change that. This commit replaces the HEAD class's partially successful attempt to annotate `commit` as `self.commit` in `__init__` with a fully recognized annotation for `commit` in the class scope (which is interpreted as an instance attribute). Merely removing the annotation in `__init__` is sufficient to make the mypy error for it go away, but this causes the inferred type of `x.commit` when `x` is a `HEAD` instance to be inferred as Any rather than the desired specific type Commit. That is why this also adds an annotation to the class to achieve that without causing its own mypy error as the old one did. Once the SymbolicReference.commit property has a more specific static type, however that is to be achieved, there will be no need for the annotation added in the HEAD class body here. (This differs from the situation in 3aeef46 before this, where Diffable does not inherit any `repo` attribute--and is intended not to--and therefore had to introduce an annotation for it, for mypy to accept it.)
Configuration menu - View commit details
-
Copy full SHA for f1cc1fe - Browse repository at this point
Copy the full SHA f1cc1feView commit details -
Broaden cygpath parameter annotation
To fix a mypy error in Repo.__init__ where the epath variable, which can sometimes be a os.PathLike[str], does not match the str annotation for cygpath's path parameter, even though cygwin immediately calls str on that parameter. Pulling most of cygpath out into a new helper function is to help mypy correctly infer that the type of path is still compatible with the return type of str, when it is used in the return statement. I'm not sure this change is really the best solution at this time, because while it fixes the mypy error (without creating a new one), and cygpath is not listed in __all__, the docstring of advises to call Git.polish_url instead of cygpath. That method is public, annotates its parameter as str, and should not have its annotation broadened without considernig if it makes sense to do so or if its docstring needs to be updated. So either the cygpath docstring should be updated or this change should be undone and a different approach used to fix the type error in Repo.__init__.
Configuration menu - View commit details
-
Copy full SHA for e133018 - Browse repository at this point
Copy the full SHA e133018View commit details -
Have Repo.__init__ convert epath to str first instead
In e133018 before this, I had noticed that Repo.__init__ implicitly relied on the implementation detail of cygpath that it converts its argument to str immediately. That detail was not reflected in the parameter type annotation, which could've been broader, so I had fixed the mypy error in Repo.__init__ by broadening cygpath's parameter type annotation. This undoes that change, putting the cygpath annotations back as before, and fixes the mypy error in another way, by having Repo.__init__ pass `str(epath)` instead of `epath` to cygpath. The reason is the concern noted in the e133018 commit message, that broadening the annotation made the way cygpath documents its relationshp to Git.polish_url no longer correct, and that it is not clear that the str(path) step in cygpath really ought to be made more than an implementation detail (which if done may require some documentation to be changed). Instances of str are usually direct instances (rather than of a subclass of str), in which case an extra call to str will (at least in CPython, and probably in all implementations) just return the same str object that was passed in. The performance penalty of this extra call to str should therefore be extremely small.
Configuration menu - View commit details
-
Copy full SHA for c34a466 - Browse repository at this point
Copy the full SHA c34a466View commit details -
Fix how Remote annotates dynamic config-backed url attribute
This fixes a mypy error by replacing an annotation on `self.url`, which is a mypy error, by adding an annotation at the class level for a `url` instance attribute. This adds a corresponding brief docstring for it as well, which may slightly improve usability, but the main impact is one less mypy error.
Configuration menu - View commit details
-
Copy full SHA for 4dfd480 - Browse repository at this point
Copy the full SHA 4dfd480View commit details -
Drop wrong variable annotations in BlobFilter.__call__
This fixes two mypy errors by removing the annotations of the filter_parts and blob_parts local variables as lists. They only need to be sequences (more precisely, they only need to be sized and iterable, they don't even need to support subscripting in the way __call__ uses them), and mypy is able to infer their type.
Configuration menu - View commit details
-
Copy full SHA for e4fd2e3 - Browse repository at this point
Copy the full SHA e4fd2e3View commit details -
Clarify CallableProgress vs. CallableRemoteProgress
This corrects an overstatement in the git.types.CallableProgress docstring, which was introduced recently in 9e47083, and in which I had erroneously claimed that it was the most general type of object passed as a progress reporter for cloning. There are some non-None non-callable types that can also be passed and that are not encompassed by git.types.CallableProgress, somewhat confusingly including CallableRemoteProgress (which like RemoteProgress is not callable; rather, it wraps a callable and forwards progress information to it). In addition, None can be passed, and while git.types.CallableProgress does encompass it, it is not callable. (This is minor by comparison and I just added a brief note for it.) This also further expands the git.types.CallableProgress docstring, as well as the git.util.CallableRemoteProgress docstring, to clarify the distinction between them, as well as what "Callable" really signifies for CallableRemoteProgress (that it wraps and forwards to a callable, rather than itself being callable).
Configuration menu - View commit details
-
Copy full SHA for 94344b4 - Browse repository at this point
Copy the full SHA 94344b4View commit details -
Fix RootModule.update
ignore[override]
suppressionIt was on the `previous_commit` parameter, and ineffective. It may be that the original intent of the suppression was to suppress only incompatible override type errors due to the addition of that parameter, but there are other paramters that also mismatch by having a different name or by being absent even though present in the overridden base-class method. Furthermore, even coresponding parameters mismatch in position due to the insertion of the `previous_commit` parameter, so even if there is some way to do a more fine-grained suppression than applying `ignore[override]` to the entire method signature, that would be insufficient here. This fixes one mypy error. It does so by causing it to be suppressed effectively, not by fixing the underlying issue, which may not be fixable since it would entail a breaking change to fix. However, this does not introduce any new suppressions, just fixes an existing suppression so it is effective (and probably so it operates as intended, though maybe it is slightly stronger than intended as discussed above).
Configuration menu - View commit details
-
Copy full SHA for 8e8b87a - Browse repository at this point
Copy the full SHA 8e8b87aView commit details -
Fix wrong class name in git.objects.tag docstring
In 6126997 (gitpython-developers#1850), I had meant to have the git.objects.tag module docstring say that the module defined the TagObject class, to help ensure no one would confuse this with the TagReference class. But I instead had it wrongly say it defined the TagReference class! This fixes that.
Configuration menu - View commit details
-
Copy full SHA for 1cdec7a - Browse repository at this point
Copy the full SHA 1cdec7aView commit details -
Correct and clarify Diffable.diff docstring
In cd16a35 (gitpython-developers#1725), I had taken "Treeish" to mean the type of that exact name, git.index.base.Treeish. But that type is only used within the git.index package (actually only in git.index.base itself). It is also nonpublic: git.index.base.__all__ exists and does not list it. So most likely this was not intended in the git.diff.Diffable.diff docstring. Even if intended, it does not appear accurate, since the git.index.base.Treeish union includes bytes, and the logic in Diffable.diff and its helpers does not appear to accommodate bytes. A closer type is the public git.types.Tree_ish union, which is narrower than git.index.base.Treeish, including neither str nor bytes. However, it does not include str, and Diffable.diff does accept str to specify a tree-ish for diff-ing. It may be that "Treeish" in the pre-gitpython-developers#1725 docstring was capitalized for some reason other than to identify a type defined in GitPython's code. For now, I've changed it to refer to git.types.Tree_ish, but also explicitly documented that a string can be used to specify a tree-ish -- which is independently valuable, since previously the effect of passing a str instance to the diff method was not stated anywhere in the method docstring. To clarify further, I included a link to tree-ish in gitglossary(7) as well. In addition, the original wording before cd16a35 had included "(type)", which I had erroneously assumed was just meant to state that it is a type (i.e. a class), so I had wrongly removed it without replacing it with anything when making it into a reference to a type. But it was really an attempt to clarify that Diffable.Index should be used directly, rather than an instance of it. That is in effect the opposite of merely pointing out that it is a class; it is to express that it should be used in a way that does not depend in any way on it being a class. This commit has the docstring explicitly state that.
Configuration menu - View commit details
-
Copy full SHA for ed6ead9 - Browse repository at this point
Copy the full SHA ed6ead9View commit details
Commits on Mar 9, 2024
-
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.
Configuration menu - View commit details
-
Copy full SHA for 0e1df29 - Browse repository at this point
Copy the full SHA 0e1df29View commit details -
Consolidate str and os.PathLike[str] (use GitPython's PathLike)
Where they had been introduced and written separately in 0e1df29 before this. The main reason is that it would have to be quoted as a string for compatibility with Python 3.8 and lower, since os.PathLike only defined __class_getitem__ in 3.8 and later. (The definition in git.types already does that.) In addition, the benefit of keeping them separate was negligible, as mentioned in 0e1df29.
Configuration menu - View commit details
-
Copy full SHA for 62c0823 - Browse repository at this point
Copy the full SHA 62c0823View commit details -
Further clarify Diffable.diff docstring
Along the lines of ed6ead9.
Configuration menu - View commit details
-
Copy full SHA for 7204cc1 - Browse repository at this point
Copy the full SHA 7204cc1View commit details -
Annotate _process_diff_args without Diffable.Index
This removes the static type of Diffable's nested Index class, Type["Index"], from the Diffable._process_diff_args method, and its override IndexFile._process_diff_args. Further changes related to this remain necessary, and at this time, this adds one mypy error. The _process_diff_args methods did not handle Diffable.Index. The base class method would pass it through, but it would not be usable when then passed to "git diff". Instead, it is handled with correct behavior at runtime in Diffable.diff and its IndexFile.diff override, which handle it themselves and ensure it is never passed to any _process_diff_args implementation. That was already the case. The change here is mostly to type hints, removing it from the _process_diff_args annotations, since those methods have never actually worked with it and it probably wouldn't make sense for them to handle it. However, this does also attempt to help mypy figure out that Diffable.Index cannot end up as an element of its local variable `args`. This attempt is unsuccessful. The problem with the `args` local variable may be the reason including Index in the parameter type annotations appeared correct or necessary before. The issue is that Diffable.Index, even though it ought to be used as an opaque constant, is a class. Its static type is Type[Diffable.Index], but mypy is unable to infer that there are no other objects of that static type, because if Diffable.Index were subclassed, and the type object produced from doing so (i.e. the subclass itself) were passed as an `other` argument to the diff method, then the `other is Diffable.Index` condition would evaluate to False. Therefore to solve this problem it should be sufficient to decorate the Diffable.Index class as `@final`. This works for pyright (and thus also pylance), but it does not work for mypy (even in 1.9.0). So that still has to be solved, and (other than via suppressions) it may be necessary to make Diffable.Index an enumeration constant rather than a class, so it can be annotated as a literal.
Configuration menu - View commit details
-
Copy full SHA for 2f5e258 - Browse repository at this point
Copy the full SHA 2f5e258View commit details -
Make NULL_TREE and Index precisely annotatable
This creates a git.diff.DiffConstants enumeration and makes the git.diff.NULL_TREE and git.diff.Diffable.Index objects constants in it. This allows them (including as an alternative in a union) to be annotated as literals: - Literal[DiffConstants.NULL_TREE] - Literal[DIffConstants.INDEX] Although the enumeration type must unfortunately be included in the annotations as shown above (at least mypy requires this), using the objects/values themselves does not require any code to change. So this shouldn't break anything at runtime for code using GitPython, unless it has relied on NULL_TREE being a direct instance of object, or relied on Diffable.Index (all useful uses of which were also as an opaque constant) being defined as a class. More specifically, all the ways that NULL_TREE and Index could be *accessed* before are still working, because: - NULL_TREE is aliased at module level, where it was before. It is also aliased at class level in Diffable, for consistency. - INDEX is aliased at class level in Diffable, as Index, where it was before. This way, Diffable.Index can still be used. It is also aliased, in the same scope, as INDEX. Because it is, and in effect has always been, a constant, the new INDEX spelling is preferable. But there is no major disadvantage of the old Index spelling, so in docstrings I have made no effort at this time to discourage its use. (If GitPython ever uses all-caps identifiers strictly as constants, then the clarity benefit of ensuring only the INDEX version is used will be greater, and then it might make sense to deprecate Index. However, this seems unlikely to happen as it would be a breaking change, due to the way functions like git.reset rebind Git.GIT_PYTHON_GIT_EXECUTABLE.) INDEX is also aliased at module level, for consistency. - NULL_TREE is still included in git.diff.__all__, causing it to be recognized as public in git.diff and also to be accessible as an attribute of the top-level git module (which currently uses wildcard imports). For consistency, I have also included INDEX in __all__. - Because there is a benefit to being able to freely referene the new DiffConstants enumeration in annotations (though in practice this may mostly be to implementers of new Diffable subclasses), I have also included DiffConstants in __all__. In addition, the name DiffConstants (rather than, e.g., Constants) should avoid confusion even if it ends up in another scope unexpectedly. To avoid a situation where users/developers may erroneously think these aliases are different from each other, I have documented the situation in the docstrings for each, referring to the others. (Sphinx does not automatically use the original docstring for an aliased name introduced in this way, and there is also arguably a clarity benefit to their differing wording, such as how each refers *only* to the others.) Other docstings are also updated. This commit completes the change begun in 2f5e258 before this, resolving the one mypy error it added. But this does not complete the larger change begun in 0e1df29: - One of the effects of this change is to make it possible to annotate precisely for NULL_TREE, either by using Literal[DiffConstants.NULL_TREE] or consolidating it with the Literal[DiffConstants.INDEX] alternative by including DiffConstants in the union instead of either/both of them. - But that is not yet done here, and when it is done for Diffable.diff, the LSP issue in IndexFile.diff, which does not currently accommodate NULL_TREE, will resurface (or, rather, be rightly revealed by mypy, in a way that is specifically clear).
Configuration menu - View commit details
-
Copy full SHA for 65863a2 - Browse repository at this point
Copy the full SHA 65863a2View commit details -
Configuration menu - View commit details
-
Copy full SHA for c9952e1 - Browse repository at this point
Copy the full SHA c9952e1View commit details -
Modify annotations to accommodate NULL_TREE
This finishes the main changes being done at this time to the annotations of diff-related methods that were started in 0e1df29. As expected and noted in previous commits, having Diffable.diff permit NULL_TREE entails a violation of the Liskov substitution principle in the overridden method IndexFile.diff unless a similar change were also made there, which could not be done correctly without modifying its behavior to actually accept NULL_TREE (it does not contain code to cover it, and raises ane exception if it is passed). I am unsure if that should ultimately be done, but even if so, it seems to me to be beyond the scope of the typing changes being done here. This therefore applies a suppression there. The suppression is specific to that one parameter. The long-standing comment atop the IndexFile.diff method, which reads as vague today, is replaced with a specific FIXME comment describing the situation where the method refers to the base-class docstring for documentation of its parameters yet doesn't accept NULL_TREE (and notes the mypy error). In effect this is really restoring and fixing the suppression that was present before 0e1df29 rather than adding a "new" one, but at that time the base-class parameter type was much broader since it was a union with object as one of its alternatives, so the situation was much less clear.
Configuration menu - View commit details
-
Copy full SHA for b8a25df - Browse repository at this point
Copy the full SHA b8a25dfView commit details -
Add refresh to top-level __all__
This makes the git.refresh function unambiguously public. git.refresh was already public in the sense that it was explicitly documented as appropriate to call from code outside GitPython. However, it had not been included in git.__all__. Because __all__ existed but omitted "refresh", git.refresh had appeared non-public to automated tools. This also does some cleanup: - It removes a comment that showed how git.__all__ had been defined dynamically before gitpython-developers#1659, since with the addition of "refresh", git.__all__ no longer contains exactly the same elements as that technique produced (as it examined the module's contents prior to running the def statement that bound the name "refresh"). - With that comment removed, it is no longer necessary to define __all__ below the imports to show what the dynamic techinque had operated on. So this moves it up above them in accordance with PEP-8.
Configuration menu - View commit details
-
Copy full SHA for e49327d - Browse repository at this point
Copy the full SHA e49327dView commit details -
Deprecate public access to typing imports in git
This adds comments to entries in git.__all__ for each of the entries that come from the standard library typing module, noting them as deprecated. These imports were included in __all__ inadvertently due to the way __all__ was dynamically constructed, and placed in __all__ explicitly when __all__ became static in gitpython-developers#1659. They are there for backward compatibility, in case some code relies on them being there. But a module is unlikely to rely intentionally on the git module providing them, since they are not conceptually related to GitPython. `from git import *` should not typically be used, since wildcard imports are not generally recommended, as discussed in PEP-8. But if someone does choose to use it, they would probably benefit less from DeprecationWarning being issued for each of those names than they would usually benefit from DeprecationWarning. This could lead to developers deciding not to enable DeprecationWarning when it may otherwise be useful. For this reason, no attempt is currently made to issue DeprecationWarning when those names are accessed as attributes of the git module.
Configuration menu - View commit details
-
Copy full SHA for c8ad3a3 - Browse repository at this point
Copy the full SHA c8ad3a3View commit details -
Mention collections.abc for Sequence
In the deprecation comment for importing Sequence from the top level git module. Although the git module imports it from typing, collections.abc.Sequence supports __class_getitem__ since Python 3.9 (and typing.Sequence is actually itself deprecated since then).
Configuration menu - View commit details
-
Copy full SHA for 3c8cbe9 - Browse repository at this point
Copy the full SHA 3c8cbe9View commit details -
Add INDEX and DiffConstants to git.__all__
This adds the names of git.diff.INDEX and git.diff.DiffConstants to the top-level __all__. In particular, the recently introduced INDEX constant has the same standing as NULL_TREE, which was (rightly) already listed, and this change makes true the claims in some docstrings in the git.diff module that say that INDEX is accessible (among other ways) as git.INDEX.
Configuration menu - View commit details
-
Copy full SHA for 87b314e - Browse repository at this point
Copy the full SHA 87b314eView commit details -
Adjust mypy options to work well with mypy 1.9.0
mypy 1.9.0 was very recently released. This makes two changes, in light of that, while avoiding breaking earlier versions: - Change the Python version mypy assumes from 3.7 to 3.8 as configiured in pyproject.toml, because starting in 1.9.0 mypy no longer supports type checking for Python 3.7. It makes sense to keep having a low version checked on local runs of mypy (when not further overridden with `--python-version`). Since the CI mypy steps currently are written not to fail their jobs if they fail (and they do fail, as there remain some mypy errors that are neither fixed nor suppressed), mypy status from CI is not obvious, and it may be that the main way mypy is actually used when developing GitPython is locally. But keeping it down to 3.7 prints a warning and falls back to the version of Python that is used to run mypy, starting in mypy 1.9.0. - Have each CI job that runs mypy do so for the Python version that job exists to test. (This is already the case for the *platform* that job exists to test, because the platform mypy should check for is not overridden in pyproject.toml.) This is done by passing the test matrix Python version with `--python-version`, which takes priority over the pyproject.toml option (which itself takes priority over the default version selection, which in this case would be the same as what we are turning it back into on CI). This is worthwhile even separately from the changes in mypy 1.9.0. But one of its effects is to make it so the mypy step in the Python 3.7 test jobs still checks Python 3.7 (so the results for 3.7, which GitPython has not yet dropped support for) can still be examined. This works becuase the latest version of mypy that can *run* on 3.7, which pip selects automatically, *does* support type-checking for 3.7.
Configuration menu - View commit details
-
Copy full SHA for 9ed904c - Browse repository at this point
Copy the full SHA 9ed904cView commit details -
Colorize mypy output on CI for easier reading
See: - python/mypy#13815 - python/mypy#13817 This seems to be working in in all jobs *except* for the Python 3.7 and 3.8 jobs on macOS.
Configuration menu - View commit details
-
Copy full SHA for aeacb00 - Browse repository at this point
Copy the full SHA aeacb00View commit details
Commits on Mar 10, 2024
-
Remove some unneeded mypy suppressions
These include suppressions that are no longer required because they worked around mypy bugs that have been fixed, or because they were not actually effective. It may also include some that are no longer needed becuase of improvements made to GitPython's type annotations (or other code) since they were introduced, I am not sure. This deliberately keeps warn_unused_ignores uncommented in pyproject.toml, at least for now.
Configuration menu - View commit details
-
Copy full SHA for 84fc806 - Browse repository at this point
Copy the full SHA 84fc806View commit details -
show_error_codes is deprecated, and represents the default now.
Configuration menu - View commit details
-
Copy full SHA for 96ecc2e - Browse repository at this point
Copy the full SHA 96ecc2eView commit details -
Apply intended suppression in Tree.traverse
The superclass _traverse method call in Tree.traverse appears to have once had a working suppression for the incompatbile types of two arguments, `predicate` and `prune`, as well as for an argument that required (and requires) no suppression, `depth`. These three arguments were written on the same line, which bad a `type: ignore` comment on it. But when black formatting was applied in 21ec529 (gitpython-developers#1442), that comment moved so that it was on a line with just the `depth` call that didn't need it, rather than the others that did. Since then, mypy has reported errors, which further seem intended to suppress based on the surrounding context and the use of `cast` to deal with the static type incompatibilities going the other way. This misplaced suppression was one of the ones I very recently removed in 84fc806. But really there should be a suppression for those arguments (at least for now, while the code remains written that way, given that a suppression is intended). This suppresses the error effectively by inserting two suppression comments, one for each of the two arguments. This is more specific than a single suppression applying to the whole call, and keeping the arguments on separate lines both makes black happy and makes clear that it is not by coincidence that the error is suppressed for both of them. The new suppressions are also written for the specific mypy error at issue, rather than fully general as before. This change decreases the number of mypy errors by two.
Configuration menu - View commit details
-
Copy full SHA for 97d9b65 - Browse repository at this point
Copy the full SHA 97d9b65View commit details -
Spell self.Index as self.INDEX in IndexFile.diff
Because, as noted in 65863a2, it is now (slightly) preferable to write INDEX. Note that this is solely stylstic: they are equivalent and must remain so, so that existing code that uses GitPython and accesses it as Index (including in overridden diff methods when subclassing Diffable) continues to work. This changes it in the exception message as well.
Configuration menu - View commit details
-
Copy full SHA for ad00c77 - Browse repository at this point
Copy the full SHA ad00c77View commit details -
Test that redefined Diffable.Index should be compatible
This tests the most important characteristics of Diffable.Index that must be (and are) preserved in making it an alias of the enumeration constant INDEX instead of a nested class definition. Adding this additional test also allows commit.INDEX to be used in place of commit.Index in the existing test (since part of what this tests is that they are the same object). That change is also made, along with some very minor cleanup of immediately nearby test code.
Configuration menu - View commit details
-
Copy full SHA for 2decbe4 - Browse repository at this point
Copy the full SHA 2decbe4View commit details -
Have git module use sys.platform to check for Windows
This changes the way code throughout the git module checks to see if it is running on a native Windows system. Some checks using os.name were already changed to use sys.platform in dc95a76 and 4191f7d. (See dc95a76 on why the specific question of whether the system is native Windows can be answered by either checking os.name or sys.platform, even though in general they differ in granularity and are not always suited to the same tasks.) The reasons for this change are: - Consistency: Due to dc95a76 and 4191f7d, some of these checks use sys.platform, so in the absence of a reason to do otherwise, it is best that they all do. Otherwise, it creates an appearance that the technical reasons behind the difference are stronger than they really are, or even that differnt things are being checked. - Better static typing: mypy treats sys.platform as a constant (and also allows checking on platform on another via --platform). Likewise, typeshed conditions platform-dependent declarations on sys.platform checks, rather than os.name or other checks. Really this is the original reason for those earlier, more selective changes, but here the goal is more general, since this is not needed to address any specific preexisting mypy errors. This is incomplete, for two reasons: - I'm deliberately not including changes in the tests in this commit. Arguably it should be kept as os.name in the tests, on the grounds that the test suite is not currently statically typed anyway, plus having them differ more compellingly shows that the behavior is the same whether an os.name or sys.platform check is used. However, it would be confusing to keep them different, and also somewhat unnatural; one approach would probably end up leaking through. Furthermore, because some tests have to check for cygwin specifically, which cannot be done with os.name, it may be clearer to use sys.platform for all platform checking in the test suite. But to verify and demonstrate that the change really is safe, I'm waiting to make the change in the tests until after making them in the code under test. - Some forms of checks against constants produce unreachable code errors from mypy (python/mypy#10773). This can be worked around, but I have not done that in this commit. Furthermore, the new mypy errors this produces--one on Windows, and one on non-Windows systems--could be fixed in a way that makes type annotations richer, by allowing the return type to be a literal on one platform or the other.
Configuration menu - View commit details
-
Copy full SHA for 88557bc - Browse repository at this point
Copy the full SHA 88557bcView commit details -
Fix new mypy error in _read_win_env_flag
As noted in 88557bc before this, that change added a couple of new mypy errors about unreachable code, where it is intentionally unreachable because it is for one platform and not another. This fixes one of them, though a fix that incorporates more of what can now be statically known about the return value based on the platform may be preferable.
Configuration menu - View commit details
-
Copy full SHA for 7204c13 - Browse repository at this point
Copy the full SHA 7204c13View commit details -
Configuration menu - View commit details
-
Copy full SHA for 42e10c0 - Browse repository at this point
Copy the full SHA 42e10c0View commit details -
Have test suite use sys.platform to check for Windows
See 88557bc on this change in general as well as the rationale for making it in the tests. (It is alreay done in the code under test.)
Configuration menu - View commit details
-
Copy full SHA for 465ab56 - Browse repository at this point
Copy the full SHA 465ab56View commit details -
Wrap docstrings and comments in _safer_popen_windows
It gained an indentation level in dc95a76, so its docstrings and comments were no longer wrapped to 88 columns as most docstrings and comments have been (absent a reason not to) since gitpython-developers#1850. This wraps it, but some parts may benefit from some other adjustments.
Configuration menu - View commit details
-
Copy full SHA for ad8190b - Browse repository at this point
Copy the full SHA ad8190bView commit details -
Further improve _safer_popen_windows doc
This adjusts the formatting of the _safer_popen_windows docstring and comments, and rewords some parts for clarity and brevity.
Configuration menu - View commit details
-
Copy full SHA for b9d9e56 - Browse repository at this point
Copy the full SHA b9d9e56View commit details -
Temporarily rename Commit_ish to Old_commit_ish
And Lit_commit_ish to Lit_old_commit_ish. This is temporary, and only for purposes of bookkeeping while this type is split into two types. Specifically, Old_commit_ish will go away soon, because each occurrence of it will be changed to one of: - A newly redefined Commit_ish union of types representing only git object types that are sometimes commit-ish. - A newly introduced union (using some name formerly not used in GitPython) of all four types representing git object types, the same types as the old Commit_ish, here renamed Old_commit_ish, had overbroadly covered. - Perhaps in some cases something else. For context, see gitpython-developers#1858 (including comments), and comments in gitpython-developers#1859.
Configuration menu - View commit details
-
Copy full SHA for 04a2753 - Browse repository at this point
Copy the full SHA 04a2753View commit details -
Define and document AnyGitObject and (new) Commit_ish
These provide for the changes planned in 04a2753 before this (at least the first two cases listed). But annotations still need to be changed to use these types.
Configuration menu - View commit details
-
Copy full SHA for 787f65c - Browse repository at this point
Copy the full SHA 787f65cView commit details -
Define GitObjectTypeString and update Object to use it
This is equivalent to the old Lit_commit_ish union; it is the type of a literal string that has one of the four values that represent actual git object types. The old Lit_commit_ish union was only used in one place in GitPython: to annotate Object.type. This replaces that and updates its docstring, as well as the Object class docstring. (One change in the Object class docstring--adding a mention of the RootModule subclass of Submodule--is not conceptually related to these other changes.)
Configuration menu - View commit details
-
Copy full SHA for 1fe4dc8 - Browse repository at this point
Copy the full SHA 1fe4dc8View commit details -
Configuration menu - View commit details
-
Copy full SHA for 7328a00 - Browse repository at this point
Copy the full SHA 7328a00View commit details -
Configuration menu - View commit details
-
Copy full SHA for 191f4cf - Browse repository at this point
Copy the full SHA 191f4cfView commit details -
Remove extra
parents
param in Commit.__init__ docstringIt was listed earlier and was incorrect, not corresponding to the position or type of this initializer's actual `parents` parameter.
Configuration menu - View commit details
-
Copy full SHA for d1ce940 - Browse repository at this point
Copy the full SHA d1ce940View commit details
Commits on Mar 11, 2024
-
Help tools know the type of a Commit's
parents
mypy seems not to need this, and already infers the specific type List[Commit], but some other type checkers -- at least pylance, and thus probably also pyright -- do not infer this.
Configuration menu - View commit details
-
Copy full SHA for fe42ca7 - Browse repository at this point
Copy the full SHA fe42ca7View commit details -
Keep the type of a Commit's
parents
from being too narrowThe type should be Sequence[Commit], rather than List[Commit] as some type checkers seem already to have been inferring and as some others were caused to infer by the indirect fix that precedes this.
Configuration menu - View commit details
-
Copy full SHA for e66297a - Browse repository at this point
Copy the full SHA e66297aView commit details -
Fix remaining old Commit_ish annotations in git.repo.fun
Some were replaced in 191f4cf (which also included other fixes). The fixes here consist mostly of replacing the remaining uses of the old Commit_ish, though some now-unneeded casting is also removed.
Configuration menu - View commit details
-
Copy full SHA for fe7f9f2 - Browse repository at this point
Copy the full SHA fe7f9f2View commit details -
Fix remaining old Commit_ish annotations in git.refs
(This also improves the sorting of imports where they are already being changed, and fixes a typo in a comment.)
Configuration menu - View commit details
-
Copy full SHA for ab27827 - Browse repository at this point
Copy the full SHA ab27827View commit details -
Fix IndexFile.commit
parent_commits
annotationThis changes that parameter's annotation to be equivalent to the annotation of the corresponding Commit.create_from_tree parameter. It had been annotated as the old Commit_ish type, but it passes that argument to Commit.create_from_tree, and it also refers to that method in its docstring for usage. Commit.create_from_tree annotates this argument as an optional list of Commit objects, and if the objects obtained when it iterates them are not Commit instances, then an exception is raised. Even if being able to pass other commit-ish things (or references, strings, etc.) is desirable, that does not currently work. This also improves sorting (and grouping style) of imports in that file (which were being changed already to no longer import the old Commit_ish type).
Configuration menu - View commit details
-
Copy full SHA for b4b6e1e - Browse repository at this point
Copy the full SHA b4b6e1eView commit details -
Fix old Commit_ish annotations in git.remote
The combination of changes here is a bit unintuitive. - In PushInfo.__init__, the old_commit parameter is an optional string, whose value is bound to the _old_commit_sha instance attribute. The old_commit property attempts to create a Commit object using this string. When _old_commit_sha is None, this property returns None. Otherwise it calls Repo.commit on the Repo object for the PushInfo's associated remote. Repo.commit should return, and is annotated to return, a Commit object. Its return value is produced by calling rev_parse, which is actually the implementation in git.fun, which always returns an Object (and more specifically an AnyGitObject) and whose annotation was recently fixed in fe7f9f2. The way rev_parse is used appears to only be able to get a Commit, but even if it were to get something else, it would still be an Object and not a SymbolicReference or string. Therefore, the return annotation, which contained the old Commit_ish, was overly broad, in part because the old Commit_ish was defined over-broadly, but also in other ways. This changes it to declare that it returns a Commit object or None, not allowing any kind of refs, strings, or instances representing git objects other than commits. The new return annotation is also what type checkers infer for the operand of the return statement, ever since git.fun.rev_parse's own return annotation was fixed. - In contrast, the situation with FetchInfo is almost the opposite. FetchInfo.__init__ had declared its old_commit parameter as being of the old Commit_ish type or None. Given the name old_commit, this may seem at first glance to be a case where only actually commit-ish values should be accepted, such that the annotation may have been overly broad due the overbroad old definition of Commit_ish. But on closer inspection it seems that may not be so. Specifically, when "git fetch" reports the refs it updated, and a ref points to something that is not commit-ish, the "old_commit" can be something that is not a commit. In particular, if a remote lightweight tag points to a blob or tree (rather than the usual case of pointing to a commit or tag), and that tag is then changed, then a fetch -- if done with flags that allow it to be reset -- should result in an "old_commit" of the blob or tree. More specifically, in FetchInfo._from_line (in the "tag update" case), in a line with ".." or "...", old_commit is set by parsing a field with repo.rev_parse, which is git.fun.rev_parse, which will return an Object that may be any of the four types. This is later passed as the old_commmit parameter when constructing a FetchInfo instance. Since a lightweight tag is a ref that can refer to any of the four types, any could end up being parsed into the old_commit attribute. This therefore keeps those as broad as before, channging their old Commit_ish annotations to AnyGitObject rather than to the newer Commit_ish type or anything narrower. Note also that it is pure coincidence that entities named old_commit were temporarily annotated as Old_commit_ish. (The latter, as noted in 04a2753, is just what the old Commit_ish type was temporarily renamed to.)
Configuration menu - View commit details
-
Copy full SHA for 5b2869f - Browse repository at this point
Copy the full SHA 5b2869fView commit details
Commits on Mar 14, 2024
-
Start on fixing Submodule parent_commit annotations
These used the old Commit_ish. They may not all be the same as each other, since they perform different validation and some will look up a commit while others do not. In particular, this is complicated by how Submodile.__init__ documents its parent_commit parameter: it says to see the set_parent_commit method. But __init__ sets the _parent_commit attribute to parent_commit, while set_parent_commit treats its commit parameter differently, calling self.repo.commit on it to get get a Commit object from it even if it wasn't one to begin with. See gitpython-developers#1869 for full details. There may be some other subtleties as well. A number of uses of the of the old Commit_ish type appear in git.submodule.base, all related to parent_commit. These are all remaining references to it (identifiable due to the rename to Old_commit_ish done in 04a2753), though. So once the changes begun here for git.submodule.* are done, that will also have completed the broader replacement effort begun in 7328a00. At that point cleanup can be done in git.types (removing Old_commit_ish introduced in 04a2753, replacing Lit_old_commit_ish with a possibly-updated and deprecated Lit_commit_ish to avoid a breaking change, and possibly making further refinements to additions to the newly added docstrings).
Configuration menu - View commit details
-
Copy full SHA for 1541c62 - Browse repository at this point
Copy the full SHA 1541c62View commit details -
Fix other submodule.base parent_commit annotations
This finishes the work in git.objects.submodule.base begun in 1541c62. Some remaining occurrences of the old Commit_ish type are in git.objects.submodule.root, which still need to be replaced. Even once that is done, this will not have fixed gitpython-developers#1869, though it will be somewhat mitigated because each method's annotations related to parent_commit will reflect the types that can actually be used reliably under the current implementation, even if in some cases that may not be as broad as was meant, or hoped, to be supported. I expect this may lead the way to a fix for gitpython-developers#1869, since the existing behavior and relationships will be clearer. Where the old Commit_ish annotations were changed here to "Commit", it reflects assumptions found by examining the implementation that other alternatives are not usable becuse something beloning to a Commit is used and no conversion or fallback is performed for it. Where a commit is looked up (which uses rev_parse), broader types are annotated. Where the argument is used without conversion and must have a `tree` attribute, it must be a Commit. Where the argument could be broader but must be compared meaningfully with something known to be a commit, it must at minimum have a binsha attribute to allow that equality comparison, implemented in Object, to be meaningful, so in such cases its type cannot include str.
Configuration menu - View commit details
-
Copy full SHA for 1f03e7f - Browse repository at this point
Copy the full SHA 1f03e7fView commit details -
Fix old Commit_ish annotation in RootModule
This finishes the annotation fixup begun in 7328a00. (The temporary Old_commit_ish type is now ready to be removed.)
Configuration menu - View commit details
-
Copy full SHA for e66b8f1 - Browse repository at this point
Copy the full SHA e66b8f1View commit details -
Configuration menu - View commit details
-
Copy full SHA for 93d19dc - Browse repository at this point
Copy the full SHA 93d19dcView commit details -
Fix and deprecate Lit_commit_ish
It would be a breaking change to keep Lit_commit_ish undefined, so this brings it back, but with a narrowed definition for consistency with the corrected Commit_ish, and deprecated, with instructions on what to use instead. (This also improves import sorting in the same module.)
Configuration menu - View commit details
-
Copy full SHA for ebcfced - Browse repository at this point
Copy the full SHA ebcfcedView commit details -
Make some broad mypy suppressions more specific
- Change every bare `# type: ignore` into `# type: ignore[reason]`, where the specific reason is verified by mypy. - Use separate `# type: ignore[X]` and `# type: ignore[Y]` for different parts of statement where a single bare suppression was doing double duty, with each suppression only on the specific parts (splitting the statement into multiple lines for this). - Use the same suppression twice on very long statements where only specific parts need suppressions. This also formats all these comments with the same spacing (most but not all already were). This makes them easier to grep for.
Configuration menu - View commit details
-
Copy full SHA for b070e93 - Browse repository at this point
Copy the full SHA b070e93View commit details -
Configuration menu - View commit details
-
Copy full SHA for 0b99041 - Browse repository at this point
Copy the full SHA 0b99041View commit details -
Configuration menu - View commit details
-
Copy full SHA for 011cb0a - Browse repository at this point
Copy the full SHA 011cb0aView commit details -
Configuration menu - View commit details
-
Copy full SHA for 74f3c2e - Browse repository at this point
Copy the full SHA 74f3c2eView commit details -
Use LBYL for imports where EAFP is a mypy type error
The conditional imports of Literal from either typing or typing_extensions have to be done with if-else on the Python version rather than with try-except, or it is a static type error. This makes that change, checking sys.version_info. (See discussion in gitpython-developers#1861 and gitpython-developers#1862 for broader context and background on why this logic, before and after this change, is repeated across multiple modules.) This also reorders/regroups imports for consistency in some places, especially where a new import of the sys module (for version_info) would otherwise exacerbate inconsistency. Since the merge commit 0b99041, the number of mypy errors had increased from 5 to 10. This fixes all the new mypy errors, so the count is back to 5.
Configuration menu - View commit details
-
Copy full SHA for 5778b7a - Browse repository at this point
Copy the full SHA 5778b7aView commit details