Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Description
I believe the intent ofgit.exc
is twofold:
- To publish the exceptions it defines, which it unambiguously does.
- To republish exceptions defined in
gitdb.exc
, which it...doesn't?
The problem is that the conceptually public names in a module are (basically) those listed in__all__
when__all__
is present, and since an original and continuing use of__all__
is to control what names a*
import binds, it might seem like whatever a*
import binds is considered conceptually public even without__all__
... but that is not the case.
Instead, in the absence of__all__
, names that both do not start with an_
and alsowere not introduced merely by themselves being imported are the conceptually public names. Asthe "Public and internal interfaces" section of PEP-8 says:
Imported names should always be considered an implementation detail. Other modules must not rely on indirect access to such imported names unless they are an explicitly documented part of the containing module’s API[...]
To make such a name public, it can be included in__all__
, butgit.exc
currently does not define__all__
. Does it otherwise document those names as public? Well...
Line 6 in44102f3
""" Module containing all exceptions thrown throughout the git package """ |
This is why I say the situation is ambiguous--this might be interpreted to mean that the names are public, in that they are the names of exceptions that are thrown throughout GitPython and would not otherwise be public in GitPython. But really this is a stretch, and rather than saying the situation is ambiguous, it might be more reasonable to say that they are definitely not public.
But I am pretty sure they are intended to be public. So they should be documented as such. The best way to do this is by defininggit.exc.__all__
and listing them there.
There then becomes the question of whether anything else that is currently imported byfrom git.exc import *
belongs in__all__
. I believe the answer is no. It's not reasonable for code outsidegit.exc
to rely on those being part ofgit.exc
(because they are present only due to imports, so they are nonpublic per the above-quoted rule, and the documentation ofgit.exc
does not reference them even obliquely). It is especially unreasonable for code outside thegit.exc
module to rely on receiving them from a*
import.
However, the top-level__init__.py
does just that.When it began to do it depends on one's perspective. For some time, it has been including those names (such assafe_decode
fromgit.util
) ingit.__all__
because of the waygit.__all__
was dynamically generated with a comprehension. In#1659, the comprehension was removed and replaced with a listing of everything that was found to be in__all__
.
Becausegit.__all__
did, and does, exist, and it included those names, it should continue to include them for backward compatibility, even in the case of some of them fromtyping
that hopefully no one is importing from thegit
module. However, I think no similar reasoning applies togit.exc
itself, sincegit.exc.__all__
has not existed. This is to say that I think it is always a bug to rely on picking up nonpublic names from*
imports, including ingit/__init__.py
, which is currently doing that.
To keepfrom git import *
working, as well as to keepgit.__all__
a correct statement of names guaranteed accessible as attributes ofgit
, when fixing this ingit.exc
it will also be necessary to modify the code ofgit/__init__.py
to get these names from the modules that really provide them publicly. That should be no problem, though, and I'd say it would be an improvement even by itself.
I believegit.exc
is the clearest place in GitPython where this kind of problem exists, but every attempt to suppress an unused import lint rule is a strong hint of a similar bug, so I'm pretty sure this is far from the only case. However, I'm opening an issue about this specifically because recent changes (in#1659) combine with it to create a situation where the bug could become entrenched if not addressed fairly soon. I say this because althoughgit/__init__.py
has, for quite some time, being wrongly relying on those names being present, in that it has been effectively guaranteeing their presence for future patch versions by including them in__all__
as it would be inspected by users, it is only now that this dependence can be easily discerned, and thus perhaps further relied on, by reading the code.