René's URL Explorer Experiment

Title: Replace all wildcard imports with explicit imports by EliahKagan · Pull Request #1880 · gitpython-developers/GitPython · GitHub

Open Graph Title: Replace all wildcard imports with explicit imports by EliahKagan · Pull Request #1880 · gitpython-developers/GitPython

X Title: Replace all wildcard imports with explicit imports by EliahKagan · Pull Request #1880 · gitpython-developers/GitPython

Description: Fixes #1349 This actually makes a number of related changes, several of which the reduction in mypy errors in #1859 helped in validating: All wildcard imports (from blah import *) are replaced with imports of specific names. This is the key change, with everything else done to support it, or directly facilitated by it, or conceptually related to it, or practically easy to do alongside it. Wildcard imports are inherently brittle, discouraged in PEP-8, and have already led to some difficulties for GitPython (see fc86a23 and the "Another ambiguity, possibly relevant" section of #1802). Even more importantly, this makes things easier for type checkers, especially for code that uses GitPython from the outside where common and useful configurations otherwise report errors due to the wildcards' lack of explicitness about what names they introduce. Furthermore, the combination of the mypy error reduction in #1859 with the elimination of wildcard imports makes it feasible to verify that newly introduced dynamic behavior is done in a way that avoids breaking static type checking for users of GitPython. This allows deprecation warnings on access to specific module attributes, as may be useful in git.compat, as well as a dynamically generated git.__version__ attribute, to be safely approachable. (But such now-doable enhancements are not part of this pull request.) I wrote a short script (1e5a944, f705fd6) to verify that changes to module contents were minimal and as expected, including verifying what object attributes reference, not just what the attributes' names were. Top-level names that are imported only to match what was imported before, but that are nonpublic and inadvisable to use, are commented as such where they are imported. Most of these could possibly be removed very soon. The test suite has a few new tests, to test a particularly tricky aspect of the observable behavior of imports that is easy to change accidentally but should only ever be changed intentionally (the git.util vs. git.objects.util issue). __all__ is added where absent but clearly useful, which is everywhere under git where it was absent except git.types. This was only a handful of places. git.objects.__all__, which was dynamically generated as a list comprehension--as git.__all__ was prior to #1659--is likewise replaced with a literal __all__. As detailed in the f89d065 commit message, I used deltall (see also #1875) to verify that this was the right course. The __init__ for git.objects.submodule no longer patches IndexObject and Object into git.objects.submodule.util. This had been done to avoid a circular import error, but at some point ceased to be needed. Because git.objects.submodule.util has defined __all__ for a long time, which omits them (and seems always to have omitted them), and because it also does not use them, I didn't replace that patching with any other way of getting them in there. However, it looks like it would be feasible to import them in a way that would not cause problems, if that is called for. Also related to that situation, git.objects.submodule had been the only subpackage whose __init__ didn't make names from its own submodules available directly in it. Because neither the circular import error nor the dynamic patching workaround are being done, there is no impediment to this, which improves consistency across the project and seemed to me to be what users would expect, so I included that. __all__ is now placed at the PEP-8 recommended location near the top of each module, above imports except in rare cases where that is infeasible. A number of linter suppressions are removed, most of which are no longer needed due to wildcard imports no longer being used. Some others are made more specific, including by splitting them into suppressions that apply to specific names in a from import. Some of what facilitates this is actually the switch from flake8 to Ruff in #1862. Imports across the project are fully sorted and grouped, in a consistent style that I think reflects the sensibility of the style that has been in place for an extended time. This is something that I had done bits of as part of other pull requests, and it was convenient to "complete" it here. I use scare quotes because it is likely I have missed some things. Ideally this should eventually be checked--or, better, applied--by an automated tool. Ruff can sort imports but I don't think it supports anything like this style, while isort is more configurable but might still be difficult to get to do it. However, the goal here is not this specific style, but instead to have something as consistent and helpful as possible if the style is not changed or before it is changed, while also making it easier to identify the advantages and disadvantages of an automatically applicable style over the best the current style has to offer. Minor cleanup alongside these changes, including removal of old commented-out code (done in its own commits so it's easy to find in case that code is wanted again) and rewording the git.objects.util module docstring to distinguish that module from git.util. The latter is the reason for that __doc__ attribute change. A few special considerations: It was suggested that #1349 could be fixed by writing git.__all__ literally rather than dynamically generating it. But that improvement was since made in #1659, and while #1656 was fixed, #1349 remained. I've re-verified that this is the case by creating a repository that uses GitPython in a way that more closely approximates installing it from PyPI than is common during development, and ensures that the non-default mypy configuration in pyproject.toml is not used, while still allowing arbitrary files to be tested, by doing a non-editable installation into a different directory with a separate virtual environment. More details are in this gist. This confirmed that, as of the main branch at 0a609b9, #1349 was reproducible, while at the tip of this feature branch, d524c76, it was resolved. I am not sure what the best choice is for whether intermediate subpackages--Python modules like git.objects and git.refs that are under the top-level git package but contain their own Python submodules--should include their Python submodules' names in __all__. In GitPython (though not all projects), everything is imported as a consequence of importing the top-level git module, so these are already available, and they are also already public, in the sense that they are available due to having been imported, with the fully qualified names they are guaranteed to have as a consequence of being imported. In most but not all cases--git.objects being a major counterexample due to its previously dynamically built __all__ that omitted modules--these were already importable in wildcard imports due to the absence of __all__. However, that this was the case had caused some problems, including several modules that are not direct children of the top-level git module ending up as attributes of it (most of which are simply private attributes that should not be used, but one of which, being git.index.util, is the cause of the git.util/git.index.util ambiguity). Something different could be done in git.objects than elsewhere for maximum similarity to the previous state of affairs, but I decided instead to omit them everywhere, based on the top-level git module not listing its own direct Python submodules in its __all__, and the possibility that omitting them would help mitigate the risks of wildcard imports outside of GitPython. Some modules have unused imports not listed in __all__, present intentionally for use in other GitPython modules but not public outside the project. As commented on #1854, I'm unsure whether or how that should ultimately be improved, but I definitely consider it beyond the scope of this pull request. As such, implicit_reexport = true remains set for mypy in pyproject.toml (mypy otherwise warns about this situation wherever other modules use such imports).

Open Graph Description: Fixes #1349 This actually makes a number of related changes, several of which the reduction in mypy errors in #1859 helped in validating: All wildcard imports (from blah import *) are replaced wi...

X Description: Fixes #1349 This actually makes a number of related changes, several of which the reduction in mypy errors in #1859 helped in validating: All wildcard imports (from blah import *) are replaced wi...

Opengraph URL: https://github.com/gitpython-developers/GitPython/pull/1880

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width