Skip to content

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

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

Merged
merged 82 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
82 commits
Select commit Hold shift + click to select a range
f83b056
Revise assert_never
EliahKagan Mar 2, 2024
01cc8e2
Fix unnecessarily long reference in Tree docstrings
EliahKagan Mar 3, 2024
6f3a20f
Change how tree[subscript] is introduced
EliahKagan Mar 3, 2024
85889cd
Refine how tree[subscript] is introduced
EliahKagan Mar 3, 2024
9e47083
Start adding docstrings to types in git.types
EliahKagan Mar 3, 2024
3bd8177
Document Tree_ish, Commit_ish, and related types
EliahKagan Mar 3, 2024
f3b9a69
Expand docs of classes representing Git objects
EliahKagan Mar 3, 2024
2af7640
Do a bit of tidying related to unused names
EliahKagan Mar 3, 2024
2aa053e
Add docstrings to TypedDicts in git.types
EliahKagan Mar 3, 2024
15d50de
Revise a couple new docstrings for clarity
EliahKagan Mar 4, 2024
7166703
Fix possible inaccuracy in Lit_config_levels docstring
EliahKagan Mar 4, 2024
1530fd2
Use phrases like "git object type" where applicable
EliahKagan Mar 4, 2024
2e02b09
Add docstrings to protocols in git.types
EliahKagan Mar 4, 2024
012d710
Move our PathLike below even TYPE_CHECKING imports
EliahKagan Mar 4, 2024
a06f1fc
Remove commented-out is_config_level function
EliahKagan Mar 4, 2024
c93e431
Expand git.compat docstring
EliahKagan Mar 4, 2024
29443ce
Add a cationary note about Object vs. object
EliahKagan Mar 4, 2024
b6e3ad2
Don't bind unused _assertion_msg_format
EliahKagan Mar 4, 2024
b5d9198
Remove commented-out code
EliahKagan Mar 6, 2024
2212ac9
Fix Sphinx reference that rendered overly long
EliahKagan Mar 7, 2024
3c5ca52
Simplify _safer_popen_windows "if shell" logic
EliahKagan Mar 7, 2024
43b7f8a
Annotate safer_popen broad enough for all platforms
EliahKagan Mar 7, 2024
dc95a76
Fix mypy error with creationflags in subprocess module
EliahKagan Mar 7, 2024
4191f7d
Refactor kill_after_timeout logic so mypy can check it
EliahKagan Mar 7, 2024
1ef3365
Factor communicate and watchdog logic to helper
EliahKagan Mar 7, 2024
4083dd8
Fix new mypy confusion about kill_after_timeout type
EliahKagan Mar 7, 2024
3aeef46
Fix how Diffable annotates expected repo attribute
EliahKagan Mar 7, 2024
f1cc1fe
Fix how HEAD annotates inherited commit property
EliahKagan Mar 8, 2024
e133018
Broaden cygpath parameter annotation
EliahKagan Mar 8, 2024
c34a466
Have Repo.__init__ convert epath to str first instead
EliahKagan Mar 8, 2024
4dfd480
Fix how Remote annotates dynamic config-backed url attribute
EliahKagan Mar 8, 2024
e4fd2e3
Drop wrong variable annotations in BlobFilter.__call__
EliahKagan Mar 8, 2024
94344b4
Clarify CallableProgress vs. CallableRemoteProgress
EliahKagan Mar 8, 2024
8e8b87a
Fix RootModule.update `ignore[override]` suppression
EliahKagan Mar 8, 2024
1cdec7a
Fix wrong class name in git.objects.tag docstring
EliahKagan Mar 8, 2024
ed6ead9
Correct and clarify Diffable.diff docstring
EliahKagan Mar 8, 2024
0e1df29
Start fixing diff and _process_diff_args type annotations
EliahKagan Mar 9, 2024
62c0823
Consolidate str and os.PathLike[str] (use GitPython's PathLike)
EliahKagan Mar 9, 2024
7204cc1
Further clarify Diffable.diff docstring
EliahKagan Mar 9, 2024
2f5e258
Annotate _process_diff_args without Diffable.Index
EliahKagan Mar 9, 2024
65863a2
Make NULL_TREE and Index precisely annotatable
EliahKagan Mar 9, 2024
c9952e1
Fix Sphinx references; give Diffable.Index a docstring
EliahKagan Mar 9, 2024
b8a25df
Modify annotations to accommodate NULL_TREE
EliahKagan Mar 9, 2024
e49327d
Add refresh to top-level __all__
EliahKagan Feb 24, 2024
c8ad3a3
Deprecate public access to typing imports in git
EliahKagan Feb 24, 2024
3c8cbe9
Mention collections.abc for Sequence
EliahKagan Mar 5, 2024
87b314e
Add INDEX and DiffConstants to git.__all__
EliahKagan Mar 9, 2024
9ed904c
Adjust mypy options to work well with mypy 1.9.0
EliahKagan Mar 9, 2024
aeacb00
Colorize mypy output on CI for easier reading
EliahKagan Mar 9, 2024
84fc806
Remove some unneeded mypy suppressions
EliahKagan Mar 10, 2024
96ecc2e
Drop deprecated mypy option
EliahKagan Mar 10, 2024
97d9b65
Apply intended suppression in Tree.traverse
EliahKagan Mar 10, 2024
ad00c77
Spell self.Index as self.INDEX in IndexFile.diff
EliahKagan Mar 10, 2024
2decbe4
Test that redefined Diffable.Index should be compatible
EliahKagan Mar 10, 2024
88557bc
Have git module use sys.platform to check for Windows
EliahKagan Mar 10, 2024
7204c13
Fix new mypy error in _read_win_env_flag
EliahKagan Mar 10, 2024
42e10c0
Fix new mypy error in is_cygwin_git
EliahKagan Mar 10, 2024
465ab56
Have test suite use sys.platform to check for Windows
EliahKagan Mar 10, 2024
ad8190b
Wrap docstrings and comments in _safer_popen_windows
EliahKagan Mar 10, 2024
b9d9e56
Further improve _safer_popen_windows doc
EliahKagan Mar 10, 2024
04a2753
Temporarily rename Commit_ish to Old_commit_ish
EliahKagan Mar 10, 2024
787f65c
Define and document AnyGitObject and (new) Commit_ish
EliahKagan Mar 10, 2024
1fe4dc8
Define GitObjectTypeString and update Object to use it
EliahKagan Mar 10, 2024
7328a00
Start fixing annotations that used the old Commit_ish
EliahKagan Mar 10, 2024
191f4cf
Fix some annotations in git.repo.fun
EliahKagan Mar 10, 2024
d1ce940
Remove extra `parents` param in Commit.__init__ docstring
EliahKagan Mar 10, 2024
fe42ca7
Help tools know the type of a Commit's `parents`
EliahKagan Mar 11, 2024
e66297a
Keep the type of a Commit's `parents` from being too narrow
EliahKagan Mar 11, 2024
fe7f9f2
Fix remaining old Commit_ish annotations in git.repo.fun
EliahKagan Mar 11, 2024
ab27827
Fix remaining old Commit_ish annotations in git.refs
EliahKagan Mar 11, 2024
b4b6e1e
Fix IndexFile.commit `parent_commits` annotation
EliahKagan Mar 11, 2024
5b2869f
Fix old Commit_ish annotations in git.remote
EliahKagan Mar 11, 2024
1541c62
Start on fixing Submodule parent_commit annotations
EliahKagan Mar 11, 2024
1f03e7f
Fix other submodule.base parent_commit annotations
EliahKagan Mar 14, 2024
e66b8f1
Fix old Commit_ish annotation in RootModule
EliahKagan Mar 14, 2024
93d19dc
Remove the temporary Old_commit_ish type
EliahKagan Mar 14, 2024
ebcfced
Fix and deprecate Lit_commit_ish
EliahKagan Mar 14, 2024
b070e93
Make some broad mypy suppressions more specific
EliahKagan Mar 14, 2024
0b99041
Merge branch 'main' into doc-types
EliahKagan Mar 14, 2024
011cb0a
Apply Ruff auto-fixes not included in merge
EliahKagan Mar 14, 2024
74f3c2e
Help Ruff avoid a very long line
EliahKagan Mar 14, 2024
5778b7a
Use LBYL for imports where EAFP is a mypy type error
EliahKagan Mar 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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).
  • Loading branch information
EliahKagan committed Mar 9, 2024
commit 65863a28c2bfebe084e1c86d409fbf68b0b33a76
102 changes: 79 additions & 23 deletions git/diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# This module is part of GitPython and is released under the
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/

import enum
import re

from git.cmd import handle_process_output
Expand All @@ -22,13 +23,12 @@
Match,
Optional,
Tuple,
Type,
TypeVar,
Union,
TYPE_CHECKING,
cast,
)
from git.types import Literal, PathLike, final
from git.types import Literal, PathLike

if TYPE_CHECKING:
from .objects.tree import Tree
Expand All @@ -48,10 +48,55 @@
# ------------------------------------------------------------------------


__all__ = ("Diffable", "DiffIndex", "Diff", "NULL_TREE")
__all__ = ("DiffConstants", "NULL_TREE", "INDEX", "Diffable", "DiffIndex", "Diff")

NULL_TREE = object()
"""Special object to compare against the empty tree in diffs."""

@enum.unique
class DiffConstants(enum.Enum):
"""Special objects for :meth:`Diffable.diff`.

See the :meth:`Diffable.diff` method's ``other`` parameter, which accepts various
values including these.

:note:
These constants are also available as attributes of the :mod:`git.diff` module,
the :class:`Diffable` class and its subclasses and instances, and the top-level
:mod:`git` module.
"""

NULL_TREE = enum.auto()
"""Stand-in indicating you want to compare against the empty tree in diffs.

Also accessible as :const:`git.NULL_TREE`, :const:`git.diff.NULL_TREE`, and
:const:`Diffable.NULL_TREE`.
"""

INDEX = enum.auto()
"""Stand-in indicating you want to diff against the index.

Also accessible as :const:`git.INDEX`, :const:`git.diff.INDEX`, and
:const:`Diffable.INDEX`, as well as :const:`Diffable.Index`. The latter has been
kept for backward compatibility and made an alias of this, so it may still be used.
"""


NULL_TREE: Literal[DiffConstants.NULL_TREE] = DiffConstants.NULL_TREE
"""Stand-in indicating you want to compare against the empty tree in diffs.

See :meth:`Diffable.diff`, which accepts this as a value of its ``other`` parameter.

This is an alias of :const:`DiffConstants.NULL_TREE`, which may also be accessed as
:const:`git.NULL_TREE` and :const:`Diffable.NULL_TREE`.
"""

INDEX: Literal[DiffConstants.INDEX] = DiffConstants.INDEX
"""Stand-in indicating you want to diff against the index.

See :meth:`Diffable.diff`, which accepts this as a value of its ``other`` parameter.

This is an alias of :const:`DiffConstants.INDEX`, which may also be accessed as
:const:`git.INDEX` and :const:`Diffable.INDEX`, as well as :const:`Diffable.Index`.
"""

_octal_byte_re = re.compile(rb"\\([0-9]{3})")

Expand Down Expand Up @@ -84,7 +129,7 @@ class Diffable:
compatible type.

:note:
Subclasses require a repo member, as it is the case for
Subclasses require a :attr:`repo` member, as it is the case for
:class:`~git.objects.base.Object` instances. For practical reasons we do not
derive from :class:`~git.objects.base.Object`.
"""
Expand All @@ -94,9 +139,25 @@ class Diffable:
repo: "Repo"
"""Repository to operate on. Must be provided by subclass or sibling class."""

@final
class Index:
"""Stand-in indicating you want to diff against the index."""
NULL_TREE = NULL_TREE
"""Stand-in indicating you want to compare against the empty tree in diffs.

See the :meth:`diff` method, which accepts this as a value of its ``other``
parameter.

This is the same as :const:`DiffConstants.NULL_TREE`, and may also be accessed as
:const:`git.NULL_TREE` and :const:`git.diff.NULL_TREE`.
"""

INDEX = Index = INDEX
"""Stand-in indicating you want to diff against the index.

See the :meth:`diff` method, which accepts this as a value of its ``other``
parameter.

This is the same as :const:`DiffConstants.INDEX`, and may also be accessed as
:const:`git.INDEX` and :const:`git.diff.INDEX`.
"""

def _process_diff_args(
self,
Expand All @@ -112,7 +173,7 @@ def _process_diff_args(

def diff(
self,
other: Union[Type["Index"], "Tree", "Commit", str, None] = Index,
other: Union[Literal[DiffConstants.INDEX], "Tree", "Commit", str, None] = INDEX,
paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
create_patch: bool = False,
**kwargs: Any,
Expand All @@ -125,20 +186,15 @@ def diff(

* If ``None``, we will be compared to the working tree.

* If :class:`~git.types.Tree_ish`, it will be compared against the
respective tree. (See https://git-scm.com/docs/gitglossary#def_tree-ish.)
This can also be passed as a string.
* If a :class:`~git.types.Tree_ish` or string, it will be compared against
the respective tree.

* If :class:`Diffable.Index`, it will be compared against the index. Use the
type object :class:`Index` itself, without attempting to instantiate it.
(That is, you should treat :class:`Index` as an opqaue constant. Don't
rely on it being a class or even callable.)
* If :const:`INDEX`, it will be compared against the index.

* If :attr:`git.NULL_TREE <NULL_TREE>`, it will compare against the empty
tree.
* If :const:`NULL_TREE`, it will compare against the empty tree.

This parameter defaults to :class:`Diffable.Index` (rather than ``None``) so
that the method will not by default fail on bare repositories.
This parameter defaults to :const:`INDEX` (rather than ``None``) so that the
method will not by default fail on bare repositories.

:param paths:
This a list of paths or a single path to limit the diff to. It will only
Expand Down Expand Up @@ -185,7 +241,7 @@ def diff(
paths = [paths]

diff_cmd = self.repo.git.diff
if other is Diffable.Index:
if other is INDEX:
args.insert(0, "--cached")
elif other is NULL_TREE:
args.insert(0, "-r") # Recursive diff-tree.
Expand Down Expand Up @@ -218,7 +274,7 @@ def diff(


class DiffIndex(List[T_Diff]):
R"""An Index for diffs, allowing a list of :class:`Diff`\s to be queried by the diff
R"""An index for diffs, allowing a list of :class:`Diff`\s to be queried by the diff
properties.

The class improves the diff handling convenience.
Expand Down
5 changes: 2 additions & 3 deletions git/index/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,10 @@
Sequence,
TYPE_CHECKING,
Tuple,
Type,
Union,
)

from git.types import Commit_ish, PathLike
from git.types import Commit_ish, Literal, PathLike

if TYPE_CHECKING:
from subprocess import Popen
Expand Down Expand Up @@ -1479,7 +1478,7 @@ def reset(
# @ default_index, breaks typing for some reason, copied into function
def diff(
self,
other: Union[Type["git_diff.Diffable.Index"], "Tree", "Commit", str, None] = git_diff.Diffable.Index,
other: Union[Literal[git_diff.DiffConstants.INDEX], "Tree", "Commit", str, None] = git_diff.INDEX,
paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
create_patch: bool = False,
**kwargs: Any,
Expand Down
2 changes: 0 additions & 2 deletions git/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
TypedDict,
Protocol,
SupportsIndex as SupportsIndex,
final,
runtime_checkable,
)
else:
Expand All @@ -31,7 +30,6 @@
SupportsIndex as SupportsIndex,
TypedDict,
Protocol,
final,
runtime_checkable,
)

Expand Down