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
Since #1659, git.__all__ is written literally, rather than being dynamically constructed based on what has been defined at the time it is bound. However, the fix was based on what __all__ had been being populated with at the time, and therefore inherited two weaknesses:
Lots of imports that don't make sense to use from the git module (because they are from the standard library) are included. This is a fairly minor issue, but I suggest marking them as deprecated. Probably DeprecationWarning should not be raised when accessing them, though, because although people should not use wildcard imports in general, such warnings when using them in a REPL could obscure more important deprecation warnings.
git.refresh isn't public, at least not by the convention that when __all__ exists everything excluded from it is non-public. This is mitigated by the references in the documentation that say to use it and to prefer it to the Git.refresh method, and by its inclusion in the autodoc-generated API reference page, but it would still be best for it to be listed in git.__all__. The reason it isn't is that, back when __all__ had been constructed dynamically, this was above the def for refresh.
I've included a fix for this in #1859, since I was adding to __all__ for #1874 already. Because PEP-8 recommends __all__ come before most code in a module, and the reason that couldn't be done before was that it was being built dynamically and so had to be defined below everything that was to be included in it, I went ahead and did that too. As with #1874, in view of the grown scope of #1859, it seemed clearer to have an issue for this that #1859 lists as fixing, rather than jam it into comments there.
The other aspect of the situation with git.__all__ following #1659 is the question of whether its content really has been stable. After all, it was constructed from wildcard imports from modules whose contents sometimes changed and not all of which had __all__ attributes, and so potentially could have changed heavily across releases. If it had, then it might be acceptable to remove things that are only there for compatibility (because: compatibility with what?) and also would be necessary to add at least a commented warning when making other substantial changes, to avoid obscuring the issue.
Fortunately, that is not the case. It has been remarkably stable. I wrote a script to check and the results can be checked here. (I actually did this around the time of #1659, but there was nothing to raise an alarm about, and the matter didn't come up again until now.)
The text was updated successfully, but these errors were encountered:
I am incredibly impressed by the work you are willing to take on in order to prove correctness or validate assumptions, and of course the ingenuity of the solution that is used to do that!
Since #1659,
git.__all__
is written literally, rather than being dynamically constructed based on what has been defined at the time it is bound. However, the fix was based on what__all__
had been being populated with at the time, and therefore inherited two weaknesses:git
module (because they are from the standard library) are included. This is a fairly minor issue, but I suggest marking them as deprecated. ProbablyDeprecationWarning
should not be raised when accessing them, though, because although people should not use wildcard imports in general, such warnings when using them in a REPL could obscure more important deprecation warnings.git.refresh
isn't public, at least not by the convention that when__all__
exists everything excluded from it is non-public. This is mitigated by the references in the documentation that say to use it and to prefer it to theGit.refresh
method, and by its inclusion in the autodoc-generated API reference page, but it would still be best for it to be listed ingit.__all__
. The reason it isn't is that, back when__all__
had been constructed dynamically, this was above thedef
forrefresh
.I've included a fix for this in #1859, since I was adding to
__all__
for #1874 already. Because PEP-8 recommends__all__
come before most code in a module, and the reason that couldn't be done before was that it was being built dynamically and so had to be defined below everything that was to be included in it, I went ahead and did that too. As with #1874, in view of the grown scope of #1859, it seemed clearer to have an issue for this that #1859 lists as fixing, rather than jam it into comments there.The other aspect of the situation with
git.__all__
following #1659 is the question of whether its content really has been stable. After all, it was constructed from wildcard imports from modules whose contents sometimes changed and not all of which had__all__
attributes, and so potentially could have changed heavily across releases. If it had, then it might be acceptable to remove things that are only there for compatibility (because: compatibility with what?) and also would be necessary to add at least a commented warning when making other substantial changes, to avoid obscuring the issue.Fortunately, that is not the case. It has been remarkably stable. I wrote a script to check and the results can be checked here. (I actually did this around the time of #1659, but there was nothing to raise an alarm about, and the matter didn't come up again until now.)
The text was updated successfully, but these errors were encountered: