Replace wildcard imports with concrete imports

[trym-b]

All from <module> import * has now been replaced by
from <module> import X, Y, ....

Contributes to #1349

[Byron]

That's such a great improvement, thanks a lot!

[trym-b]

I might have accidentally introduced a regression in this PR. The reason for this might be because of the messy way imports were handled before (functionality was exposed through nested wildcard imports) and lack of testing.

In the commit before this PR (5e73cab) the result of

import git
dir(git)

was

old = ['Actor', 'AmbiguousObjectName', 'BadName', 'BadObject', 'BadObjectType', 'BaseIndexEntry', 'Blob', 'BlobFilter', 'BlockingLockFile', 'CacheError', 'CheckoutError', 'CommandError', 'Commit', 'Diff', 'DiffIndex', 'Diffable', 'FetchInfo', 'GIT_OK', 'Git', 'GitCmdObjectDB', 'GitCommandError', 'GitCommandNotFound', 'GitConfigParser', 'GitDB', 'GitError', 'HEAD', 'Head', 'HookExecutionError', 'IndexEntry', 'IndexFile', 'IndexObject', 'InvalidDBRoot', 'InvalidGitRepositoryError', 'List', 'LockFile', 'NULL_TREE', 'NoSuchPathError', 'ODBError', 'Object', 'Optional', 'ParseError', 'PathLike', 'PushInfo', 'RefLog', 'RefLogEntry', 'Reference', 'Remote', 'RemoteProgress', 'RemoteReference', 'Repo', 'RepositoryDirtyError', 'RootModule', 'RootUpdateProgress', 'Sequence', 'Stats', 'Submodule', 'SymbolicReference', 'TYPE_CHECKING', 'Tag', 'TagObject', 'TagReference', 'Tree', 'TreeModifier', 'Tuple', 'Union', 'UnmergedEntriesError', 'UnsupportedOperation', 'UpdateProgress', 'WorkTreeRepositoryUnsupported', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '__version__', '_init_externals', 'base', 'cmd', 'compat', 'config', 'db', 'diff', 'exc', 'fun', 'head', 'index', 'inspect', 'log', 'objects', 'os', 'osp', 'reference', 'refresh', 'refs', 'remote', 'repo', 'rmtree', 'safe_decode', 'symbolic', 'sys', 'tag', 'to_hex_sha', 'typ', 'types', 'util']

while this PR's commit (53d94b8) had this output:

new = ['Actor', 'BadName', 'Blob', 'BlobFilter', 'BlockingLockFile', 'CheckoutError', 'Commit', 'Diff', 'DiffIndex', 'FetchInfo', 'GIT_OK', 'Git', 'GitCmdObjectDB', 'GitCommandError', 'GitCommandNotFound', 'GitConfigParser', 'GitDB', 'GitError', 'Head', 'IndexEntry', 'IndexFile', 'InvalidGitRepositoryError', 'LockFile', 'NULL_TREE', 'NoSuchPathError', 'Object', 'Optional', 'PathLike', 'PushInfo', 'RefLog', 'Reference', 'Remote', 'RemoteProgress', 'RemoteReference', 'Repo', 'Stats', 'Submodule', 'SymbolicReference', 'TagReference', 'Tree', 'UnmergedEntriesError', '__all__', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', '__version__', '_init_externals', 'cmd', 'compat', 'config', 'db', 'diff', 'exc', 'index', 'inspect', 'objects', 'os', 'osp', 'refresh', 'refs', 'remote', 'repo', 'rmtree', 'sys', 'types', 'util']

The interesting part is the diff between the old and the new:

list(set(old) - set(new))
['CommandError', 'Tuple', 'Diffable', 'HEAD', 'UnsupportedOperation', 'ODBError', 'HookExecutionError', 'List', 'CacheError', 'log', 'BadObject', 'base', 'head', 'reference', 'TreeModifier', 'AmbiguousObjectName', 'typ', 'RootModule', 'symbolic', 'UpdateProgress', 'IndexObject', 'tag', 'WorkTreeRepositoryUnsupported', 'RepositoryDirtyError', 'fun', 'TYPE_CHECKING', 'InvalidDBRoot', 'BaseIndexEntry', 'RefLogEntry', 'Union', 'BadObjectType', 'RootUpdateProgress', 'ParseError', 'TagObject', 'to_hex_sha', 'safe_decode', 'Tag', 'Sequence']

Some of the lost imports should probably have been tested and been imported on import git, like TreeModifier, while others like List, Union and to_hex_sha should perhaps not be exposed?

I can help out fixing this regression, but I would like to know what should be exposed when importing git and what should not. Sorry for any inconvenience this might have caused.

[Byron]

Thanks a million for, somehow magically, returning to this PR before it becomes an issue. It's clearly something the review should have caught 😅.

I can help out fixing this regression, but I would like to know what should be exposed when importing git and what should not. Sorry for any inconvenience this might have caused.

It would be great to see another PR for this for sure, otherwise the only fix I could make is to revert the previous PR. I'd say that the intend was to import only what's defined in the modules the * is importing from. By now this is most certainly a bug that much code relies on, so when fixing it I'd play it safe and re-introduce all imports, even the ones that very clearly seem unintended.

Doing so seems to be the only way to pacify mypi and avoid breakage.

[trym-b]

It would be great to see another PR for this for sure, otherwise the only fix I could make is to revert the previous PR. I'd say that the intend was to import only what's defined in the modules the * is importing from. By now this is most certainly a bug that much code relies on, so when fixing it I'd play it safe and re-introduce all imports, even the ones that very clearly seem unintended.

I agree, the best way forward now is to revert the PR, and also probably add a test that verifies that the result of dir(git) has not changed. After that such a test had been added there might be a chance to remove the wildcard imports.

I have also seen that two submodules (https://github.com/gitpython-developers/smmap and https://github.com/gitpython-developers/gitdb) used by Gitpython also have the issue of doing wildcard imports that might mess with mypy, so perhaps it would be best to start with those first.

I will revert this commit asap. Thanks for the feedback

[Byron]

I agree, the best way forward now is to revert the PR, and also probably add a test that verifies that the result of dir(git) has not changed. After that such a test had been added there might be a chance to remove the wildcard imports.

That's a very good idea! Loving it :).

I have also seen that two submodules (https://github.com/gitpython-developers/smmap and https://github.com/gitpython-developers/gitdb) used by Gitpython also have the issue of doing wildcard imports that might mess with mypy, so perhaps it would be best to start with those first.

Another great catch! I'd love if these wouldn't cause additional issues or if there would be an easy way, but if not, PRs to them as well as new releases can be made for them, too.