EliahKagan
commented
1 month ago Should support for building documentation on Python 3.7 be removed now, or kept in for a while? This would make the documentation step in the CI workflow conditional on not being 3.7, and would allow:
-sphinx >= 7.1.2, < 7.2 ; python_version >= "3.8"
+sphinx >= 7.1.2, < 7.2
-sphinx == 4.3.2 ; python_version < "3.8"
-sphinxcontrib-applehelp >= 1.0.2, <= 1.0.4 ; python_version < "3.8"
-sphinxcontrib-devhelp == 1.0.2 ; python_version < "3.8"
-sphinxcontrib-htmlhelp >= 2.0.0, <= 2.0.1 ; python_version < "3.8"
-sphinxcontrib-qthelp == 1.0.3 ; python_version < "3.8"
-sphinxcontrib-serializinghtml == 1.1.5 ; python_version < "3.8"
sphinx_rtd_theme
sphinx-autodoc-typehintsI don't think this is urgent, since special-casing 3.7 can remain in place for a while, possibly as long as we otherwise test/support 3.7.
However, I also suspect not much is gained by continuing to support building documentation on 3.7, because:
- No one developing GitPython is going to only have access to Python 3.7.
- The current situation (since this PR) already special-cases 3.7, on which the generated documentation would not be quite the same (due to the much earlier major version of Sphinx).
Byron
This upgrades Sphinx from 4.3.2 to 7.1.2.
The old pinned version and its explicitly constrained dependencies are retained for now for Python 3.7 so that documentation can be built even with 3.7. (This could maybe be removed soon as a preliminary step toward eventually dropping 3.7 support.)
For Python 3.8 and higher, version 7.1.2 is used, allowing later patch versions but constrained to remain 7.1.*. This is so the same versions are likely to be selected on all Python version from 3.8 and higher, to minimize small differences in generated documentation that different versions could give, and also to simplify debugging.
The reason to upgrade Sphinx now is to support Python 3.13, which shall be (and, in the pre-releases available, is) incompatible with versions of Sphinx below 6.2. This is because those earlier Sphinx versions use the deprecated
imghdrmodule, which 3.13 removes:On old versions of Sphinx, that gives the error:
Using Sphinx 6.2 is sufficient to avoid this, but there do not seem to be any disadvantages for GitPython to remain below 7.1.2.
The reason we did not upgrade Sphinx before is that, last time we considered doing so, we ran into a problem of new warnings (that we treat as errors). This is detailed in the "Can we upgrade Sphinx?" section of #1802, especially the "What Sphinx 5 is talking about" subsection. The problem is warnings about
Actorwhen it comes in through type annotations:So this includes other changes to fix that problem as well. The solution used here is to import
Actoreven whenTYPE_CHECKINGisfalse, and write it unquoted in annotations, asActorrather than"Actor". This allows Sphinx to discern where it should consider it to be located, for the purpose of linking to its documentation.This does not incur overhead, because:
git.util, so also importingActorfromgit.utildoes not cause any modules to be imported that were not imported otherwise, nor even to be imported at a different time.git.utilhave members imported them into the top-levelgitmodule ingit.__init__whengitis imported (and thus when any Python submodule ofgitis imported).The only disadvantage is the presence of the additional name in those modules at runtime, which a user might inadvertently use and thus write brittle code that could break if it is later removed. But:
__all__and do not include"Actor"in__all__, so it is non-public.The reasons for the approach taken here, rather than any of several others, were:
I did not write out the annotations as
git.util.Actorto resolve the ambiguity because annotations should, and for some uses must, also be interpretable as expressions. But whilefrom git.util import Actorworks and makesActoravailable,git.util.Actorcannot be used as an expression even afterimport git.util. This is because, even after such an import,git.utilactually refers togit.index.util. This is as detailed in the warnings issued when it is accessed, originally from an overly broad*import but retained because removing it could be a breaking change. Seegit/__init__.pyfor details.The reason I did not write out the annotations as
git.objects.util.Actorto resolve the ambiguity is that not all occurrences where Sphinx needed to be told which module to document it as being from were within thegit.objectsPython submodule. Two of the warnings were ingit/objects/tag.py, where annotating it that way would not be confusing. But the other two were ingit/index/base.py.Although removing
Actorfromgit.objects.util.__all__would resolve the ambiguity, this should not be done, because:git.objects.utilcontains other members that relate to it directly.The reasons I did not take this opportunity to move the contents of
git/util.pyto a new file in that directory and makegit/util.pyre-export the contents, even though this would allow a solution analogous to (1) but for the new module to be used, while also simplifying importing elsewhere, were:git/util.pyhas a number of members that it makes available for use throughout GitPython but that are deliberately omitted from__all__and are meant as non-public outside the project. One approach would be to make a module with a leading_for these "internal" members, and another public ones with everything else. But that also cannot be decided based on the considerations that motivate the changes here.HIDE_WINDOWS_KNOWN_ERRORS, which is public ingit/util.pyand which currently does have an effect, will need to be considered. Although it cannot be re-bound by assigning togit.util.HIDE_WINDOWS_KNOWN_ERRORSbecause thegit.utilsubexpression would evaluate togit.index.util, there may be code that re-binds it in another way, such as by accessing the module throughsys.modules. Unlike functions and classes that should not be monkey-patched from code outside GitPython's test suite anyway, this attribute may reasonably be assigned to, so it matters what module it is actually in, unless the action of assigning to it elsewhere is customized dynamically to carry over to the "real" place.An alternative solution that may be reasonable in the near future is to modify
reference.rstso duplicate documentation is no longer emitted for functions and classes that are defined in one place but imported and "re-exported" elsewhere. I suspect this may solve the problem, allowing theActorimports to go back underif TYPE_CHECKING:and to annotate with"Actor"again while still runningmake -C doc htmlwith no warnings.However, this would entail design decisions about how to still document those members. They should probably have links to where they are fully documented. So an entry for
Actorin the section ofreference.rstforgit.objects.utilwould still exist, but be very short and refer to the full autodoc item forActorthe section forgit.util. Since a:class:reference togit.objects.util.Actorshould still go to the stub that links togit.util.Actor, it is not obvious that solving the duplication in documentation generated forreference.rstought to be done in a way that would address the ambiguity Sphinx warns about, even if it can be done in such a way.Therefore, that should also be a separate consideration and, if warranted, a separate change.
This builds successfully:
continue-on-errorto observe the documentation step, though that could be done if necessary. Here's a CI run including 3.13.0rc1 without upgrading Sphinx, and here's a CI run with including 3.13.0rc1 as well as the changes from this PR.Notice how in the API reference each section name on the left side is now expandable, and the items inside it are then expandable, and so forth. This adds another way of finding what one is looking for. I didn't deliberately change anything here to achieve that; it comes along with the version upgrade.