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

fix: Parse Deprecated sections in Google-style docstrings #353

Merged
merged 5 commits into from
Jan 17, 2025
Merged

fix: Parse Deprecated sections in Google-style docstrings #353

merged 5 commits into from
Jan 17, 2025

Conversation

the-13th-letter
Copy link
Contributor

@the-13th-letter the-13th-letter commented Jan 16, 2025 •
edited
Loading

Fixes #352.

This was referenced Jan 16, 2025
Closed
Closed
@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

Looks good! Was there something else you wanted to do before getting out of draft?

@the-13th-letter
Copy link
Contributor Author

Had an IRL appointment coming up, and it was evident that if something went wrong, I'd need to return to it later in the evening (i.e., now).

There's two things I still want to address:

  1. I'd like to embed the issue number in the tests for context, not just in the commit that introduces the tests. At worst, it's redundant. At best, it saves you a git bisect run.
  2. There are some quality check complaints. One of them is my fault (src/_griffe/docstrings/google.py:713:48: Q000 Single quotes found but double quotes preferred), and I will take care of that one, but the others are all from code I didn't touch. Do you want me to handle these as well?

@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

1

👍

2

You can run make format for the single vs. double quotes.

No need to handle the other unrelated quality errors, I'll deal with them in another commit/PR 🙂

@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

I've pushed a fix for the quality job, it should now pass. You can merge main or rebase on it, as you prefer.

@the-13th-letter
tests: Add tests for Google-style Deprecated sections
3cf96c5 
These tests currently fail, because the Deprecated sections are parsed
as admonitions.

Issue-352: #352
@the-13th-letter
fix: Parse Deprecated sections in Google-style docstrings
259a8fe 
The code to parse these sections already existed, but the "Deprecated"
section header was missing from the table of known section headers.
Being an unknown section, Griffe then treated this as an admonition.

Issue-352: #352
@the-13th-letter
fix: Parse Deprecated sections without version info in Google-style d…
16da63e 
…ocstrings

Griffe requires a version indicator in Google-style Deprecated sections.
If no such indicator is found, then the user is warned, and *then* the
section **is silently dropped**.

[As a proponent of readable docstrings][1], [and apparently in spirit
with the Google style guide][2], I consider it unacceptable to drop
information from a docstring just because it is not properly formatted
as data.

In this patch, I propose parsing the section without an associated
version number instead, and using the section's text as its... well,
text.

[1]: mkdocstrings/python#166 (comment) '"the source form of the docstring should be readable"'
[2]: google/styleguide#117 (comment)  '"Docstrings are meant for human consumption, please keep them readable for us humans. :)"'

Issue-352: #352
@the-13th-letter the-13th-letter force-pushed the google-section-deprecated branch from 52e0835 to 16da63e Compare January 16, 2025 20:29
@the-13th-letter the-13th-letter marked this pull request as ready for review January 16, 2025 20:32
@the-13th-letter
Copy link
Contributor Author

Rebased on 1.5.5, ready for review. No formatting/linting errors. There are two failing CI checks (tests), but those both appear to be failing before the tests can even run.

pawamoy
pawamoy approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a formatting suggestion, not important.

src/_griffe/docstrings/google.py Outdated Show resolved Hide resolved
pawamoy
pawamoy requested changes Jan 17, 2025
View reviewed changes
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, lets not warn about the missing version if we allow it to be missing.

@the-13th-letter the-13th-letter force-pushed the google-section-deprecated branch from 2ce9669 to 60a777b Compare January 17, 2025 10:10
@the-13th-letter
Copy link
Contributor Author

Actually, lets not warn about the missing version if we allow it to be missing.

Done in 60a777b.

6d758e2 is a rewording of 2ce9669. I don't use Angular-style commit messages, so the "format" prefix was from memory… but apparently "style" is the correct prefix.

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2025

Thanks a lot!

I usually squash commits and rewrite the message (if needed) when merging, so don't bother too much with commit messages 🙂

@pawamoy pawamoy merged commit 3ad6333 into mkdocstrings:main Jan 17, 2025
24 of 26 checks passed
@the-13th-letter
Copy link
Contributor Author

Thanks as well!

@the-13th-letter the-13th-letter deleted the google-section-deprecated branch January 17, 2025 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawamoy pawamoy pawamoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: two issues with parsing Google-style Deprecated sections
2 participants
@the-13th-letter @pawamoy