Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unclear why fun.py (and others?) shouldn't have a module docstring #1733

Closed
EliahKagan opened this issue Nov 4, 2023 · 2 comments · Fixed by #1735
Closed

Unclear why fun.py (and others?) shouldn't have a module docstring #1733

EliahKagan opened this issue Nov 4, 2023 · 2 comments · Fixed by #1735
Labels
acknowledged help wanted

Comments

@EliahKagan
Copy link
Contributor

While working on #1725 I noticed the git.index.fun module has this comment at the top:

GitPython/git/index/fun.py

Lines 1 to 2 in d5fc6e5

# Standalone functions to accompany the index implementation and make it more versatile.
# NOTE: Autodoc hates it if this is a docstring.

Actually, that exact comment is due to my revision in that PR, but it's revised from:

GitPython/git/index/fun.py

Lines 1 to 3 in add46d9

# Contains standalone functions to accompany the index implementation and make it
# more versatile
# NOTE: Autodoc hates it if this is a docstring

This was introduced in 9ccd777, at which time an existing module docstring was converted to a comment instead. Various other module docstrings were also converted to comments. But some modules today do have docstrings. Running make -C doc html seems to work fine; I ran it a number of times on Ubuntu and Windows while working on that PR and since.

I didn't want to assume it was okay to turn that back into a docstring. But either it (and any other top of module comments) should be turned into docstrings, or the reason they should not be should be made clearer (and any module docstrings causing problems fixed or converted back to comments).
@Byron
Copy link
Member

Byron commented Nov 6, 2023

That's an interesting find and I don't have an answer as to why that was necessary then. Ideally, that would have been in the commit message that introduced the comment, but here we are.
My guess is that it's alright to change it back and see what happens, I'd expect it to work as well.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 6, 2023
@EliahKagan
Revise and restore some module docstrings
aed3b59 
In 9ccd777, several module docstrings were converted into comments
or outright deleted, apparently due to at least one of them (in
git/index/fun.py) and possibly others having broken Sphinx autodoc.

As discussed later in gitpython-developers#1733, this no longer seems to be a problem,
not even in git/index/fun.py where the comment included a note
indicating it had.

So this converts them back to docstrings, re-adding those that were
removed in 9ccd777. This also revises them, partly to avoid making
outdated claims, but mostly for style and clarity. It also revises
some already-present module docstrings. Other than modules 9ccd777
affected, this does not add new docstrings to modules without them.

One of the revisions made to some module docstrings here, to
improve readability, is to remove language like "Module
implementing" or "Module for", which was already used only in some
places. However, this language is retained in the specific cases
where it is clarifying, for example by avoiding documenting a
module as if it the same thing as a single class implemented in it
(which would be distracting because it would read as a mistake, and
which could be confusing in cases where a developer uses an import
with an "as" clause and needs to debug it because it was intended
to be an "import" or "from" statement but was accidentally written
as the other of those).
@EliahKagan EliahKagan mentioned this issue Nov 6, 2023
Merged
@EliahKagan
Copy link
Contributor Author

EliahKagan commented Nov 6, 2023 •
edited
Loading

Sounds good. I've done this in #1735, which also revises some already-present module docstrings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
acknowledged help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revise and restore some module docstrings EliahKagan/GitPython
2 participants
@Byron @EliahKagan