Improve static typing and docstrings related to git object types #1859

EliahKagan · 2024-03-04T21:19:20Z

Fixes #1857
Fixes #1858
Fixes #1866
Fixes #1873
Fixes #1874
Fixes #1875

Note: This pull request expanded considerably in scope since first opened, which its title reflects but the rest of this description does not. See comments for details.

Main changes

This expands the docstrings of Object and some of its subclasses. These revisions are, in large part, to explain the classes' relationships to the git concept of objects and to the corresponding git object types, including links to relevant definitions in gitglossary(7).

This also adds docstrings to all the types introduced in git.types--those that relate directly to git object types and those that do not. Adding documentation in git.types relates topically to the docstring expansions for Object and some of its subclasses, and I tried to make all new and edited docstrings make sense whether read by themselves or together with other docstrings across modules. It also relates to the goal for git.types documentation articulated in #1854 (comment).

Other changes

This also includes some other changes that are less core to that scope but that seem practically useful to include. It revises the docstring for assert_never, which had been the only docstring in git.types, and modifies the string of the default exception it raises to print the repr rather than the str. It removes some long-unused nonpublic names at module level, and revises some method docstrings of classes whose class docstrings were being revised. It also expands the git.compat module docstring, toward the goal for it that was also articulated in #1854 (comment).

Considerations for `assert_never`

The change to assert_never, which includes making the docstring accurately describe the current (and established) behavior of the function, may fix #1857. However, as described there I am unsure if this is ultimately the right change there.

Considerations for `Commit_ish` docstring (including possible inaccuracies)

Even more important is the relationship of the Commit_ish docstring to #1858. I have said this PR "may" fix that issue, but I strongly suspect that either further refinement will be needed or that a significantly greater change, possibly outside the scope of this PR, may be needed.

Furthermore, some of the text in that docstring may require improvement before being suitable for merging. To facilitate writing the docstrings in git.types, including related docstrings and even other parts of the Commit_ish docstring, I wrote text in two parts of the Commit_ish docstring that sound plausible but that I am not at all sure are correct.

This is in regard to the question, detailed in #1858, of why Commit_ish union includes the never-commitish Tree and Blob classes. (Even if what I wrote turns out to be correct, it may need to be clarified.) I have opened review comments on those potentially inaccurate lines.

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.

There is no advantage to fully qualifying names in the same module as the entity being documented, but I had accidentally done that.

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.

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.

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)

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.

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.)

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.

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.

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.

And the commented-out imports that had been solely to support it.

To make clear that code outside GitPython would not typically benefit from using anything in that module. See gitpython-developers#1854 for context.

See also f78587f (gitpython-developers#1725).

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.

git/types.py

Byron

Thanks so much for your help with this!

I made a few remarks, but the biggest question certainly is if Commit_ish can be fixed instead of trying to justify it in the documentation. To me it seems possible, but I may be very wrong about that.

Thanks for sharing your thoughts.

git/objects/base.py

git/types.py

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.

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.)

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.

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.)

- 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.

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.

EliahKagan · 2024-03-14T08:29:06Z

I think this is ready.

Rather than writing a lot here about the most non-obvious aspects of the change, I've opened three more issues to describe the problems they address, #1873, #1874, and #1875, linked and marked as fixed in the edited PR description.

The core changes here remain the type annotation fixes throughout, reducing the number of mypy errors to 5, and of course; the fix for #1858, and directly and indirectly related annotations; and of course the originally scoped changes in adding docstrings in git.types and expanding them elsewhere, which, due to also addressing #1858, are able to be done much better than when I had originally opened this PR.

I've commented in #1858 (comment) on naming and why I chose the name AnyGitObject. This can of course be changed without difficulty, if done before this is merged (or even after this is merged but before whenever the next release is to be). As always, I am willing to revise further in this or any other areas if that may be needed.

Byron

What an impressive attention to detail, I can hardly imagine how it's possible to comb through this many files while keeping a keen eye out on complex types, and end up improving them.

Whether perfectly accurate or not, I believe this PR marks a huge leap towards a typing that makes sense and helps with usage.

Thank you!

git/types.py

Byron · 2024-03-14T13:24:50Z

git/types.py

+    :class:`~git.objects.tree.Tree` classes, **all** of whose instances are tree-ish.
+    This has been done because of the way GitPython uses it as a static type annotation.
+
+    :class:`~git.objects.tag.TagObject`, some but not all of whose instances are


Thanks for mentioning this. It's great that this type includes what GitPython can handle though, as one day it might mean it can be taught to resolve tags as well.

I had avoided changing this since it is not inherently a wrong definition of Tree_ish, but actually the situation you are describing, where it can be broader, is already the case. Furthermore, both Commit_ish and Tree_ish in git.types make a lot more sense if they both include TagObject rather than one of them including it and the other not. I've opened #1878 to add it.

This includes a brief description of the Submodule.__init__ parent_commit parameter in its docstring, rather than only referring to the set_parent_commit method, whose semantics differ due to conversation and validation, and which accepts more types than just Commit or None. The wording is based on wording in set_parent_commit, adjusted for the difference in types, and set_parent_commit remains reference for further details. This builds on 1f03e7f (gitpython-developers#1859) in improving the situation described in gitpython-developers#1869.

The Tree_ish union omitted TagObject, whose instances are only sometimes tree-ish, and unlike Commit_ish before gitpython-developers#1859, it is not inherently a bug to define Tree_ish this way. However, this Tree_ish type actually has only one use in GitPython (which was also the case before the changes in gitpython-developers#1859): as, itself, an alternative in the union used to annotate the rev parameter of the Repo.tree method (whose other alternatives are str and None). A TagObject may be passed, and if it points to a tree or commit then that will be resolved. Just to avoid a mypy error, code doing that would (before this change) have to convert it to str first. That annotation should be improved, and the best way to do it is to keep it written the same way but change the definition of Tree_ish in git.types to include TagObject. The reason is that doing so alleviates a major unintuitive aspect of the relationship between the Commit_ish and Tree_ish types: Commit_ish was broader than everything commit-ish, while Tree_ish was narrower than everything tree-ish. I had not considered making this change in gitpython-developers#1859 because I didn't want to modify Tree_ish unnecessarily, and its definition was not inherently a bug. However, the change to Commit_ish is sufficiently large (though it only affects static typing) that a change to Tree_ish to make them coherent and intuitive may be justified. This commit changes Tree_ish so that, in addition to its Commit and Tree alternatives, it also includes TagObject. This also updates and simplifies its docstring accordingly, bringing it in line with that of Commit_ish which is already defined with the same kind of breadth, and further revises both docstrings to more explicitly clarify when tags are tree-ish or commit-ish and when they are not. This does not change the separate nonpublic Treeish type defined in git.index.base (and named with no underscore), which omits TagObject but includes bytes and str, and which is used to annotate parameters of the IndexFile.from_tree and IndexFile.merge_tree methods. Changes there may be valuable, but the goal here is just to build narrowly on gitpython-developers#1859 to address a shortcoming of the revisions to git.types.

EliahKagan · 2024-03-15T02:15:48Z

Because of its effects on type checking of code that uses the GitPython library, this PR should probably be distinguished in some way, and as such may merit highlighting per #1806 (comment). If #1878 is accepted, then it contributes to this effect on type annotations, but so long as the effect is discernible I don't think they both have to be highlighted.

This touches on some questions about the goals being achieved by marking pull requests in this way, but that is not specific to this so I'll ask about it in the #1806 comments instead.

Byron · 2024-03-15T08:23:59Z

Thanks for pointing this out - I have marked it. Please do feel free to do so yourself in future, I trust your judgement and know I am likely to forget it

EliahKagan · 2024-03-15T12:51:59Z

Now that the purpose of the highlighting is clarified, per #1806 (comment), I'll go ahead and add it where I'm aware it's applicable. in future pull requests I open.

The submodules being made explicit here are of course Python submodules, not git submodules. The git.objects.submodule Python submodule (which is *about* git submodules) is made explicit here (as are the names imported from that Python submodule's own Python submodules) along with the other Python submodules of git.objects. Unlike some other submodules, but like the top-level git module until gitpython-developers#1659, git.objects already defined __all__ but it was dynamically constructed. As with git.__all__ previously (as noted in gitpython-developers#1859), I have used https://github.com/EliahKagan/deltall here to check that it is safe, sufficient, and probably necessary to replace this dynamically constructed __all__ with a literal list of strings in which all of the original elements are included. See: https://gist.github.com/EliahKagan/e627603717ca5f9cafaf8de9d9f27ad7 Running the modattrs.py script, and diffing against the output from before the current import refactoring began, reveals two changes to module contents as a result of the change here: - git.objects no longer contains `inspect`, which it imported just to build the dynamically constructed __all__. Because this was not itself included in that __all__ or otherwise made public or used out of git.objects, that is okay. This is exactly analogous to the situation in 8197e90, which removed it from the top-level git module after gitpython-developers#1659. - The top-level git module now has has new attributes blob, commit, submodule, and tree, aliasing the corresponding modules. This has happened as a result of them being put in git.objects.__all__ along with various names imported from them. (As noted in some prior commits, there are tradeoffs associated with doing this, and it may be that such elements of __all__ should be removed, here and elsewhere.)

EliahKagan added 18 commits March 3, 2024 09:34

Fix unnecessarily long reference in Tree docstrings

01cc8e2

There is no advantage to fully qualifying names in the same module as the entity being documented, but I had accidentally done that.

Start adding docstrings to types in git.types

9e47083

Add docstrings to TypedDicts in git.types

2aa053e

Revise a couple new docstrings for clarity

15d50de

Add docstrings to protocols in git.types

2e02b09

Remove commented-out is_config_level function

a06f1fc

And the commented-out imports that had been solely to support it.

Expand git.compat docstring

c93e431

To make clear that code outside GitPython would not typically benefit from using anything in that module. See gitpython-developers#1854 for context.

Add a cationary note about Object vs. object

29443ce

See also f78587f (gitpython-developers#1725).

EliahKagan commented Mar 4, 2024

View reviewed changes

git/types.py Outdated Show resolved Hide resolved

git/types.py Outdated Show resolved Hide resolved

EliahKagan marked this pull request as ready for review March 4, 2024 21:33

This was referenced Mar 4, 2024

The API Reference documents too few modules #1854

Closed

NullHandler added to all but git.repo.base logger #1806

Closed

Byron reviewed Mar 5, 2024

View reviewed changes

git/objects/base.py Outdated Show resolved Hide resolved

git/objects/base.py Show resolved Hide resolved

git/types.py Outdated Show resolved Hide resolved

git/types.py Outdated Show resolved Hide resolved

EliahKagan mentioned this pull request Mar 6, 2024

Commit_ish is much broader than commit-ish #1858

Closed

EliahKagan added 6 commits March 6, 2024 14:09

Remove commented-out code

b5d9198

Fix Sphinx reference that rendered overly long

2212ac9

EliahKagan added 5 commits March 14, 2024 01:45

Merge branch 'main' into doc-types

0b99041

Apply Ruff auto-fixes not included in merge

011cb0a

Help Ruff avoid a very long line

74f3c2e

This was referenced Mar 14, 2024

Module docstring says TagReference but means TagObject #1873

Closed

Diffable.diff is misleadingly annotated regarding special other values #1874

Closed

Top-level refresh function not listed in __all__ #1875

Closed

EliahKagan marked this pull request as ready for review March 14, 2024 08:29

EliahKagan requested a review from Byron March 14, 2024 08:30

Byron approved these changes Mar 14, 2024

View reviewed changes

Byron merged commit bcea9a8 into gitpython-developers:main Mar 14, 2024
26 checks passed

EliahKagan deleted the doc-types branch March 14, 2024 13:53

EliahKagan mentioned this pull request Mar 14, 2024

Describe Submodule.__init__ parent_commit parameter #1877

Merged

EliahKagan mentioned this pull request Mar 15, 2024

Include TagObject in git.types.Tree_ish #1878

Merged

Byron changed the title ~~Improve static typing and docstrings related to git object types~~ 📣Improve static typing and docstrings related to git object types📣 Mar 15, 2024

EliahKagan mentioned this pull request Mar 19, 2024

Replace all wildcard imports with explicit imports #1880

Merged

Byron added the 📣highlight-in-changelog📣 Specifically highlight this PR in the changelog after it was created label Mar 19, 2024

Byron changed the title ~~📣Improve static typing and docstrings related to git object types📣~~ Improve static typing and docstrings related to git object types Mar 19, 2024

EliahKagan mentioned this pull request Mar 30, 2024

Issue and test deprecation warnings #1886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve static typing and docstrings related to git object types #1859

Improve static typing and docstrings related to git object types #1859

EliahKagan commented Mar 4, 2024 •

edited

Loading

Byron left a comment

EliahKagan commented Mar 14, 2024 •

edited

Loading

Byron left a comment

Byron Mar 14, 2024

EliahKagan Mar 15, 2024

EliahKagan commented Mar 15, 2024

Byron commented Mar 15, 2024

EliahKagan commented Mar 15, 2024

Improve static typing and docstrings related to git object types #1859

Improve static typing and docstrings related to git object types #1859

Conversation

EliahKagan commented Mar 4, 2024 • edited Loading

Main changes

Other changes

Considerations for assert_never

Considerations for Commit_ish docstring (including possible inaccuracies)

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Mar 14, 2024 • edited Loading

Byron left a comment

Choose a reason for hiding this comment

Byron Mar 14, 2024

Choose a reason for hiding this comment

EliahKagan Mar 15, 2024

Choose a reason for hiding this comment

EliahKagan commented Mar 15, 2024

Byron commented Mar 15, 2024

EliahKagan commented Mar 15, 2024

EliahKagan commented Mar 4, 2024 •

edited

Loading

Considerations for `assert_never`

Considerations for `Commit_ish` docstring (including possible inaccuracies)

EliahKagan commented Mar 14, 2024 •

edited

Loading