Skip to content

Issue and test deprecation warnings #1886

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 89 commits into from
Mar 31, 2024
Merged
Changes from 1 commit
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
2382891
Test that deprecated Diff.renamed property warns
EliahKagan Mar 27, 2024
e7dec7d
Have the deprecated Diff.renamed property issue a warning
EliahKagan Mar 27, 2024
a8f109c
Fix exception in Popen.__del__ in test on Windows
EliahKagan Mar 28, 2024
fffa6ce
Test that the preferred renamed_file property does not warn
EliahKagan Mar 28, 2024
bc111b7
Add a TODO for simplifying the single_diff fixture
EliahKagan Mar 28, 2024
e3728c3
Decompose new fixture logic better
EliahKagan Mar 28, 2024
ff4b58d
Extract no-deprecation-warning asserter as a context manager
EliahKagan Mar 28, 2024
2c52696
Test that the deprecated Commit.trailers property warns
EliahKagan Mar 28, 2024
03464d9
Have the deprecated Commit.trailers property issue a warning
EliahKagan Mar 28, 2024
9d096e0
Test that Traversable.{list_,}traverse, but not overrides, warn
EliahKagan Mar 28, 2024
21c2b72
Use the :exc: Sphinx role for DeprecationWarning
EliahKagan Mar 28, 2024
ca385a5
Test that subclassing deprecated git.util.Iterable warns
EliahKagan Mar 28, 2024
8bbcb26
Call repo.close() instead of manually collecting
EliahKagan Mar 28, 2024
b8ce990
Better name and document the basic deprecation test module
EliahKagan Mar 28, 2024
61273aa
Annotate basic deprecation tests; have mypy scan it
EliahKagan Mar 29, 2024
b7a3d8c
Start on top-level module attribute access regression tests
EliahKagan Mar 19, 2024
105f500
Test attribute access and importing separately
EliahKagan Mar 19, 2024
859e38c
Expand to test top-level deprecated names
EliahKagan Mar 21, 2024
46a739d
Hoist `import git` to module level in test module
EliahKagan Mar 21, 2024
a2df3a8
Test static typing of private module aliases
EliahKagan Mar 21, 2024
a15a830
Improve a couple test case docstrings
EliahKagan Mar 21, 2024
dbaa535
Add a couple missing assert keywords
EliahKagan Mar 21, 2024
d00c843
Clarify how test_private_module_aliases is statically checkable
EliahKagan Mar 21, 2024
983fda7
Move mark-sharing tests into a class
EliahKagan Mar 21, 2024
19acd4c
Add FIXME for what to do next
EliahKagan Mar 22, 2024
f39bbb5
Fix a test docstring
EliahKagan Mar 22, 2024
aee7078
Test resolution into git.index.util using git.util
EliahKagan Mar 22, 2024
7f4a191
Fix brittle way of checking warning messages
EliahKagan Mar 22, 2024
d08a576
Clarify todo
EliahKagan Mar 22, 2024
9d58e6d
Start reorganizing new tests more in the GitPython style
EliahKagan Mar 23, 2024
45c128b
Finish reorganizing; fix assertion for duplicated messages
EliahKagan Mar 23, 2024
247dc15
Add imports so pyright recognizes refs and index
EliahKagan Mar 23, 2024
b05963c
Expand and clarify test module docstring
EliahKagan Mar 23, 2024
074bbc7
Tiny import tweak
EliahKagan Mar 23, 2024
18608e4
Pick a better name for _MODULE_ALIAS_TARGETS
EliahKagan Mar 23, 2024
1f290f1
Use typing_extensions only if needed
EliahKagan Mar 23, 2024
7a4f7eb
Fix zip calls
EliahKagan Mar 23, 2024
5977a6e
Fix (and improve wording) of docstrings
EliahKagan Mar 23, 2024
5b1fa58
Remove extra import "from typing_extensions"
EliahKagan Mar 23, 2024
a07be0e
Start on test_compat
EliahKagan Mar 23, 2024
d4917d0
Expand to test all three is_<platform> aliases
EliahKagan Mar 23, 2024
f4e5f42
Slightly improve docstrings
EliahKagan Mar 23, 2024
d54f851
Add test of dir() on git.compat
EliahKagan Mar 23, 2024
aaf046a
Add static type assertions to is_platform test
EliahKagan Mar 23, 2024
84d734d
Refactor test_compat.test_dir for clarity
EliahKagan Mar 23, 2024
3a621b3
Add top-level dir() tests
EliahKagan Mar 23, 2024
05e0878
Remove old comment meant as todo (that was done)
EliahKagan Mar 23, 2024
3fe2f15
Test that top-level aliases point to modules with normal __name__
EliahKagan Mar 23, 2024
246cc17
Use names directly on other tests
EliahKagan Mar 23, 2024
d7b6b31
Fix a small docstring typo
EliahKagan Mar 23, 2024
96089c8
Improve description in test module docstrings
EliahKagan Mar 23, 2024
a0ef537
Start on test_types
EliahKagan Mar 24, 2024
52e7360
Explain substring assertions in test_toplevel
EliahKagan Mar 24, 2024
e3675a0
Expand Lit_commit_ish test name and write docstring
EliahKagan Mar 24, 2024
4857ff0
Clarify test_compat.test_dir
EliahKagan Mar 24, 2024
488cc13
Add test of dir() on git.types
EliahKagan Mar 24, 2024
19b3c08
Clarify comment about is_<platform> value assertions
EliahKagan Mar 24, 2024
28bd4a3
Issue warnings for some deprecated attributes of modules
EliahKagan Mar 20, 2024
dffa930
Refine deprecated module attributes and their warnings
EliahKagan Mar 24, 2024
7ab27c5
Start on test module about Git.USE_SHELL and Git attributes
EliahKagan Mar 24, 2024
af723d5
Make test_use_shell_on_class more robust
EliahKagan Mar 25, 2024
bf13888
Write most remaining Git attribute/deprecation tests
EliahKagan Mar 25, 2024
602de0c
Begin multiprocessing misadventure
EliahKagan Mar 25, 2024
d4b50c9
Somewhat clarify multiprocessing misadventure
EliahKagan Mar 25, 2024
02c2f00
Discuss multiprocessing in test module docstring; remove bad test
EliahKagan Mar 26, 2024
46df79f
Discuss metaclass conflicts in test module docstring
EliahKagan Mar 26, 2024
40ed842
Revise test module docstring for clarity
EliahKagan Mar 26, 2024
6a35261
Test that USE_SHELL is unittest.mock.patch patchable
EliahKagan Mar 27, 2024
e725c82
Make the restore_use_shell_state fixture more robust
EliahKagan Mar 27, 2024
436bcaa
Add `type: ignore` in test that we can't set USE_SHELL on instances
EliahKagan Mar 27, 2024
febda6f
Clarify unittest.mock.patch patchability test docstring
EliahKagan Mar 27, 2024
4037108
Test that Git.execute's own read of USE_SHELL does not warn
EliahKagan Mar 27, 2024
0e311bf
Suppress type errors in restore_use_shell_state _USE_SHELL branches
EliahKagan Mar 27, 2024
df4c5c0
Fix wrong/unclear grammar in test_instance_dir docstring
EliahKagan Mar 28, 2024
d38e721
Issue warnings whenever Git.USE_SHELL is accessed
EliahKagan Mar 27, 2024
05de5c0
Implement instance USE_SHELL lookup in __getattr__
EliahKagan Mar 27, 2024
04eb09c
Have USE_SHELL warn but work like normal via super()
EliahKagan Mar 27, 2024
c6f518b
Keep mypy from thinking Git has arbitrary class attributes
EliahKagan Mar 27, 2024
c5d5b16
Clarify that the private name mangling is intentional
EliahKagan Mar 27, 2024
84bf2ca
Read USE_SHELL in Git.execute without DeprecationWarning
EliahKagan Mar 27, 2024
5bef7ed
Add GitPython project top comments to new test modules
EliahKagan Mar 29, 2024
3da47c2
Hide `del util` from type checkers
EliahKagan Mar 29, 2024
bdabb21
Expand USE_SHELL docstring; clarify a test usage
EliahKagan Mar 29, 2024
b51b080
Explain the approach in test.deprecation to static checking
EliahKagan Mar 29, 2024
7cd3aa9
Make test.performance.lib docstring more specific
EliahKagan Mar 30, 2024
cf2576e
Make/use test.deprecation.lib; abandon idea to filter by module
EliahKagan Mar 30, 2024
f92f4c3
Clarify security risk in USE_SHELL doc and warnings
EliahKagan Mar 31, 2024
8327b45
Test GitMeta alias
EliahKagan Mar 27, 2024
f6060df
Add GitMeta alias
EliahKagan Mar 27, 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
Have USE_SHELL warn but work like normal via super()
This reimplements Git.USE_SHELL with no properties or descriptors.
The metaclass is still needed, but instad of defining properties it
defines __getattribute__ and __setattr__, which check for USE_SHELL
and warn, then invoke the default attribute access via super().

Likewise, in the Git class itself, a __getatttribute__ override is
introduced (not to be confused with __getattr__, which was already
present and handles attribute access when an attribute is otherwise
absent, unlike __getattribute__ which is always used). This checks
for reading USE_SHELL on an instance and warns, then invokes the
default attribute access via super().

Advantages:

- Git.USE_SHELL is again unittest.mock.patch patchable.

- AttributeError messages are automatically as before.

- It effectively is a simple attribute, yet with warning, so other
  unanticipated ways of accessing it may be less likely to break.

- The code is simpler, cleaner, and clearer. There is some
  overhead, but it is small, especially compared to a subprocess
  invocation as is done for performing most git operations.

However, this does introduce disadvantages that must be addressed:

- Although attribute access on Git instances was already highly
  dynamic, as "methods" are synthesized for git subcommands, this
  was and is not the case for the Git class itself, whose
  attributes remain exactly those that can be inferred without
  considering the existence of __getattribute__ and __setattr__ on
  the metaclass. So static type checkers need to be prevented from
  accounting for those metaclass methods in a way that causes them
  to infer that arbitrary class attribute access is allowed.

- The occurrence of Git.USE_SHELL in the Git.execute method (where
  the USE_SHELL attribute is actually examined) should be changed
  so it does not itself issue DeprecationWarning (it is not enough
  that by default a DeprecationWarning from there isn't displayed).
  • Loading branch information
EliahKagan committed Mar 29, 2024
commit 04eb09c728fbdc4501fbbba1d6f58a0bb7050470
33 changes: 15 additions & 18 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
TYPE_CHECKING,
TextIO,
Tuple,
Type,
Union,
cast,
overload,
Expand Down Expand Up @@ -336,15 +335,15 @@ class _GitMeta(type):
This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used.
"""

@property
def USE_SHELL(cls: Type[Git]) -> bool:
_warn_use_shell(False)
return cls._USE_SHELL
def __getattribute__(cls, name: str) -> Any:
if name == "USE_SHELL":
_warn_use_shell(False)
return super().__getattribute__(name)

@USE_SHELL.setter
def USE_SHELL(cls: Type[Git], value: bool) -> None:
_warn_use_shell(value)
cls._USE_SHELL = value
def __setattr__(cls, name: str, value: Any) -> Any:
if name == "USE_SHELL":
_warn_use_shell(value)
super().__setattr__(name, value)


class Git(metaclass=_GitMeta):
Expand Down Expand Up @@ -397,9 +396,7 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
"""Enables debugging of GitPython's git commands."""

_USE_SHELL: bool = False

USE_SHELL: bool
USE_SHELL: bool = False
"""Deprecated. If set to ``True``, a shell will be used to execute git commands.

Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows
Expand Down Expand Up @@ -909,6 +906,11 @@ def __init__(self, working_dir: Union[None, PathLike] = None) -> None:
self.cat_file_header: Union[None, TBD] = None
self.cat_file_all: Union[None, TBD] = None

def __getattribute__(self, name: str) -> Any:
if name == "USE_SHELL":
_warn_use_shell(False)
return super().__getattribute__(name)

def __getattr__(self, name: str) -> Any:
"""A convenience method as it allows to call the command as if it was an object.

Expand All @@ -918,11 +920,6 @@ def __getattr__(self, name: str) -> Any:
"""
if name.startswith("_"):
return super().__getattribute__(name)

if name == "USE_SHELL":
_warn_use_shell(False)
return self._USE_SHELL

return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)

def set_persistent_git_options(self, **kwargs: Any) -> None:
Expand Down Expand Up @@ -1184,7 +1181,7 @@ def execute(

stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
if shell is None:
shell = self._USE_SHELL
shell = self.USE_SHELL
_logger.debug(
"Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)",
redacted_command,
Expand Down