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

Add pydocstringformatter #4125

Merged
merged 6 commits into from
Mar 15, 2023
Merged

Add pydocstringformatter #4125

merged 6 commits into from
Mar 15, 2023

Conversation

akaszynski
Copy link
Member

In an effort to universally fix wrapping for our docstrings, I came across pydocstringformatter. It has the potential to be that solution in the future, but for the time being it's fairly limited.

I still find it quite useful at preventing simple mistakes that would normally take forever to show up in the normal documentation build. Since it does basic enforcement of numpydoc standards, let's include this.

I'm on the fence of if we should include or not include this in the blame.

@github-actions github-actions bot added the documentation Anything related to the documentation/website label Mar 13, 2023
@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #4125 (a1b0123) into main (d7f6b79) will decrease coverage by 5.29%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4125      +/-   ##
==========================================
- Coverage   95.56%   90.28%   -5.29%     
==========================================
  Files          95       95              
  Lines       20247    20247              
==========================================
- Hits        19350    18279    -1071     
- Misses        897     1968    +1071     

@adeak
Copy link
Member

adeak commented Mar 13, 2023

I'm on the fence of if we should include or not include this in the blame.

Not if we add changes to the pre-commit hooks here 😛

As far as the docs changes go, I'm also on the fence. On the one hand +61/-41 diff means this isn't significant at all, so we can just keep the diff. It's also slightly weird to see the first line of docstrings be broken up, because I thought that this was the basis of the blurb rendered on the page listing methods for a class. But apparently that doesn't seem to be the case:

Screenshot from 2023-03-13 23-57-26

("Plotter.scalar_bar - First scalar bar", missing the part about backwards compat.)

@akaszynski
Copy link
Member Author

Not if we add changes to the pre-commit hooks here

Then we must include it. Thanks.

It's also slightly weird to see the first line of docstrings be broken up

PEP257 recommends:

Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line...

Where a one-line docstring:

The docstring is a phrase ending in a period.

So, one phrase on one line. Turns out we're not aligned. We just need to provide a good "first phrase" and enforce that through CI.

@adeak
Copy link
Member

adeak commented Mar 14, 2023

Not if we add changes to the pre-commit hooks here

Then we must include it. Thanks.

I managed to write the opposite of what I meant, so thanks for figuring it out anyway.

@akaszynski akaszynski merged commit 50c5eda into main Mar 15, 2023
@akaszynski akaszynski deleted the docs/docformatter branch March 15, 2023 17:27
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Anything related to the documentation/website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants