FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix name collision by NMertsch · Pull Request #2060 · gitpython-developers/GitPython · GitHub

/ GitPython Public

Fix name collision - #2060

Merged
Byron merged 2 commits into
gitpython-developers:mainfrom
NMertsch:2023-fix-gitconfigparser-autodoc
Jul 21, 2025
ConversationCommits (2)ChecksFiles changed
Merged

Fix name collision#2060
Byron merged 2 commits into
gitpython-developers:mainfrom
NMertsch:2023-fix-gitconfigparser-autodoc

Conversation

NMertsch commented Jul 20, 2025

Copy link
Copy Markdown
Contributor

The bug

In git.config.MetaParserBuilder.__new__:

def __new__(name, bases, clsdict):  # <- `name` is the class name
    ...
    for name, method in methods:  # <- `name` is the current method name
        ...
    return super().__new__(cls, name, bases, clsdict)  # <- `name` is the name of the last method in clsdict

Downstream effects

  • git.config.GitConfigParser.__name__ is "write", not "GitConfigParser"
  • Sphinx autodoc misinterprets the nature of git.config.GitConfigParser as an alias of itself:

Fixes #2023

Byron commented Jul 20, 2025

Copy link
Copy Markdown
Member

Thanks, this PR would be great to land I am sure!

Do you have an intuition as to why CI is failing here?

NMertsch commented Jul 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Yes. There was an rst syntax error in CPython's configparser module: https://github.com/python/cpython/blob/v3.8.10/Lib/configparser.py#L768:

- `vars'
+ `vars`

configparser.RawConfigParser is the baseclass of GitConfigParser (the class which is now being documented due to the bugfix), so its get method (the one with the syntax error) is part of the public interface of GitConfigParser.

This only occurs in the pipelines for 3.8 and 3.9, so I guess it was fixed later but the fix was never backported.

I'm not sure what's the best way to handle this. I can think of multiple options, but I dislike all of them:

  1. remove -W from SPHINXOPTS in doc/Makefile to not treat warnings as errors in the whole project.
  2. modify the offending file in the Docs CI jobs before calling sphinx-build.
  3. suppressing the warning type with suppress_warnings = ["..."] (docs). There is no way to suppress warnings only for specific files or lines, so this would disable the syntax check in the whole project. Also, I can't find the warning code in the error output, but I can find that out if you choose this option. This is a docutils warning, and sphinx has no way of suppressing it.

NMertsch commented Jul 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Here is how this would render without -W (treat warnings as errors):

It is not beautiful, but people will understand it.

If I understand correctly, this project is mostly in maintenance-only mode, so the risk of such issues being newly introduced to GitPython itself is fairly low. Maybe that's the best of all the bad options?

NMertsch commented Jul 20, 2025

Copy link
Copy Markdown
Contributor Author

Sorry for causing this mess, I didn't expect any negative side effects from the bugfix.

If you want to close this PR, just go ahead, I don't have strong feeling about this and just wanted to contribute a quick win.

If you want me to investigate further in a specific direction, please let me know and I'll see what is feasible for me.

Thanks for this library, btw! :)

Byron commented Jul 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

I see - the fix now causes the configuration to be pulled in correctly, and that results in a conflict in the way a docstring is written in the library or code we pull in.

The issue itself, now that I understand it, is also amazing as it's one of these inconsistencies that shouldn't even be possible. How can a variable in a for-loop survive the for-loop, and permanently overwrite a variable in the outer scope?

Let's go with the fix, and let only errors be errors during documentation generation.

Just one more request: could you add to the body of the most recent commit why the change was made? Just copy-paste a link to the discussion here, or better yet, copy-paste the respective comment text.

Thanks again.

Here is a copilot suggestion, but it might lack detail.

Remove -W from SPHINXOPTS due to warnings triggered by quoting of 'vars'

The Sphinx build was failing with -W enabled because warnings were generated related to the way the word 'vars' is quoted in docstrings. Treating warnings as errors (-W) caused otherwise harmless docstring quoting issues to block documentation builds. Removing -W allows the build to succeed despite these warnings.

NMertsch force-pushed the 2023-fix-gitconfigparser-autodoc branch from cb77830 to 5e7d005 Compare July 20, 2025 12:15
NMertsch added a commit to NMertsch/GitPython that referenced this pull request Jul 20, 2025
Don't treat sphinx warnings as errors
5e7d005 
Workaround for python/cpython#100520 (rst syntax error in configparser docstrings), which was fixed in CPython 3.10+.
Docutils raises warnings about the invalid docstrings, and `-W` instructs sphinx to treat this as errors. We can't control or silence these warnings, so we accept them and don't treat them as errors.

See the discussion in gitpython-developers#2060 for details.
NMertsch added 2 commits July 20, 2025 14:17
Fix name collision
a4aadb0
Don't treat sphinx warnings as errors
80fd2c1 
Workaround for python/cpython#100520 (rst syntax error in configparser docstrings), which was fixed in CPython 3.10+.
Docutils raises warnings about the invalid docstrings, and `-W` instructs sphinx to treat this as errors. We can't control or silence these warnings, so we accept them and don't treat them as errors.

See the discussion in gitpython-developers#2060 for details.
NMertsch force-pushed the 2023-fix-gitconfigparser-autodoc branch from 5e7d005 to 80fd2c1 Compare July 20, 2025 12:18

NMertsch commented Jul 20, 2025

Copy link
Copy Markdown
Contributor Author

Should be good now, please let me know if there is more to do.
Thank you for the quick and kind responses, I really appreciate it.

Byron commented Jul 21, 2025

Copy link
Copy Markdown
Member

Thank you, I think the PR is ready to go now!

Byron merged commit 4d529b7 into gitpython-developers:main Jul 21, 2025
27 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Docs: No API Docs for git.config.GitConfigParser

2 participants

Back | FazBrowse Home | New Git URL