Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork941
Refactor Git.{AutoInterrupt,CatFileContentStream} nesting#2037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This makes `Git.AutoInterrupt` and `Git.CatFileContentStream`transparent aliases to top-level nonpublic `_AutoInterrupt` and`_CatFileContentStream` classes in the `cmd` module.This does not change the "public" interface. It also does notchange metadata relevant to documentation: the `__name__` and`__qualname__` attributes are set explicitly to the values they hadbefore when these classes were defined nested, so that Sphinxcontinues to document them (and to do so in full) in `Git` and as`Git.AutoInterrupt` and `Git.CatFileContentStream`.The purpose of this is to increase readability. The `Git` class isbig and complex, with a number of long members and various forms ofnesting. Since these two classes can be understood even withoutreading the code of the `Git` class, moving the definitions out ofthe `Git` class into top-level nonpublic classes will hopefullyincrease readability and help with maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot, this makes perfect sense.
Thanks also for double-checking that the documentation is still working as it did before.
Please note that I didn't validate that the removed and added portions are still the same in the interest of time.
1e463d4
intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
EliahKagan commentedJun 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No problem! It was not at all obvious to me what Sphinx would do if I had not modified the metadata. (So I tried that first, but only locally, without committing the change.) If that had turned out not to be necessary for Sphinx, then I would not be sure whether it would be best to do it or not, since I think the interface is really the same either way. In any case, I think it's better to modify the metadata like this and automatically keep the documentation, than not to do so and have Sphinx treat them as nonpublic and not emit documentation for them at all.
It looks like some common techniques don't work here, due to a combination of changed indentation, moving the code, changing the lines at the top of the code that is moved (to I've double-checked it just now by:
It looks right, with only the new, conceptually changed (e.g. adding |
This uses `TypeAlias` from the `typing` module, to make it so theassignment statments introduced ingitpython-developers#2037 (to set `Git.AutoInterrupt`and `Git.CatFileContentStream` to nonpublic module-levelimplementations `_AutoInterrupt` and `_CatFileContentStream`) aretreated by `mypy` as type aliases rather than as class variables.For details on the problem this partially fixes, seegitpython-developers#2038 and:https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliasesThe fix won't work in this form, however, because it attempts toimport `TypeAlias` unconditionally from the standard-library`typing` module, which only gained it in Python 3.10.
Uh oh!
There was an error while loading.Please reload this page.
This makes
Git.AutoInterrupt
andGit.CatFileContentStream
transparent aliases to top-level nonpublic_AutoInterrupt
and_CatFileContentStream
classes in thecmd
module.This does not change the "public" interface. It also does not change metadata relevant to documentation: the
__name__
and__qualname__
attributes are set explicitly to the values they had before when these classes were defined nested, so that Sphinx continues to document them (and to do so in full) inGit
and asGit.AutoInterrupt
andGit.CatFileContentStream
.The purpose of this is to increase readability. The
Git
class is big and complex, with a number of long members and various forms of nesting. Since these two classes can be understood even without reading the code of theGit
class, moving the definitions out of theGit
class into top-level nonpublic classes will hopefully increase readability and help with maintenance.I originally intended this as just the first commit of several. But I realized that change is probably the only one that might be controversial. So I figured I'd open a PR for it by itself.
In my view:
Git
class as far as the interface is concerned, these classes are uses by theGit
and do not themselves use theGit
class.__name__
and__qualname__
to make Sphinx continue working as desired (while still having the metadata at runtime the same as Sphinx claims) is not too big of a conceptual burden.But whether this is better is a subjective question, so I figured it would be good to get a review.
I have verified that the intended effect in Sphinx documentation--the effect that the classes continue to be presented, in full, as nested classes of
Git
, just as they may (and should!) continue to be used when they are accessed both within GitPython and by other code that uses GitPython--is what we get. Specifically:Git.AutoInterrupt
andGit.CatFileContentStream
. (They can be compared to the latest release versions of theGit.AutoInterrupt
andGit.CatFileContentStream
classes.)