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.

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

feat: numpy admonitions #219

Merged
merged 7 commits into from
Jan 15, 2024
Merged

Conversation

machow
Copy link
Contributor

@machow machow commented Oct 17, 2023

This PR addresses #214, by parsing all unknown numpydoc sections as admonitions.

I'm not totally clear on the similarity / differences between numpydoc and google style docstrings, but hopefully this makes their parsed representations a bit closer to each other.

I think one big difference is that, a well-specified numpydoc docstring is made up of two things:

  • initial description (DocstringSectionText)
  • numpydoc sections, either
    • from spec and parsed by griffe. e.g. DocstringSectionParameters
    • not parsed by griffe, represented as DocstringSectionAdmonition

However, there's one more rule:

  • When well defined numpydoc sections (e.g. DocstringSectionParameters) end, the content between them and the next section are represented with DocstringSectionText.

Here's my attempt to represent when DocstringSectionText, DocstringSectionAdmonition, and more defined sections like DocstringSectionParameters will appear.

docstring = """

I am a DocstringTextSection summary.

Parameters
------------
a: int
    the a variable
b: float
    the b variable


I am a DocstringTextSection, because the well-defined parameters section
has ended.


See Also
---------
I am an DocstringAdmonitionSection, that griffe does not parse

Abcdefslkjs
------------
I am another DocstringAdmonitionSection
"""

Happy to tweak, enhance! Would love your thoughts on whether this seems to capture what DocstringSectionAdmonition is meant for. I tried to use the google docstring code as a reference..!

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.

Looking good, thanks a lot!

I wanted to make sure that this is compatible with the allow_section_blank_line option, and upon looking at the only test (!) above that checks this option, test_empty_indented_lines_in_section_with_items, it seems to me that the option works incorrectly: lines that are only composed of spaces should be considered empty lines, and therefore the mentioned test should fail in the second part (assert len(sections2) == 1) because there are 3 empty lines between the last line of the returns section and the next non-empty line.

I'd suggest to remove that second part from the test_empty_indented_lines_in_section_with_items test, which is not meant to check blank lines, and to add some tests similar to the ones you added in this PR that make sure allow_section_blank_line works as expected. If it doesn't, we can fix it in another PR.


My reasoning about empty lines: if we consider a line with spaces only to be non-empty, users could start relying on this behavior to prevent breaking a section by keeping one or more spaces on an otherwise empty line. But formatters will probably not allow it: I think Black now removes spaces from such lines. This would clash with the perceived feature that we support non-section-breaking lines. Also users could be surprised that when both allowing or disallowing blank lines, their section do not break because of some white space that are difficult to spot. Therefore I think we should consider lines with just spaces to be empty, to avoid any ambiguity.

The current, wrong behavior probably comes from this:

        if line.startswith(4 * " "):
            ...
        elif line.startswith(" "):
            ...
        elif _is_empty_line(line):

We should probably check first if the line is empty?

The Google parser has the same conditions so it probably suffers from the same issue.

I'll send a PR to fix both parsers 🙂

UPDATE: actually the Google parser relies on indentation so the issue was mitigated. I'll still fix the order of conditions for good measure.

pawamoy added a commit that referenced this pull request Oct 18, 2023
pawamoy added a commit that referenced this pull request Jan 14, 2024
…ions on blank lines

Breaking change: This change removes the docstring option
`allow_section_blank_line` for the Numpydoc parser,
and changes the parsing logic so that blank lines are now *always* allowed,
in any number, between sections. Sections are now only delimited by
section headers themselves. The result of these changes is that:
**prose is not allowed in between Numpydoc sections anymore**,
making the parser compliant with the Numpydoc style guide and
its maintainers recommendations.

PR #220: #220
Related to PR #219: #219
Numpydoc discussion: numpy/numpydoc#463
@pawamoy
Copy link
Member

pawamoy commented Jan 14, 2024

I tried to refactor the flow a bit, to make it easier to follow. Let me know what you think if you have time. I'll probably merge tomorrow in any case 🙂

@pawamoy pawamoy merged commit 1e311a4 into mkdocstrings:main Jan 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants