You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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).
0 commit comments