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 pydocstyle to static checks #53

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add pydocstyle to static checks #53

wants to merge 1 commit into from

Conversation

tbekolay
Copy link
Member

@tbekolay tbekolay commented Aug 2, 2019

Motivation and context:
When verifying nengo/nengo#1557, it seemed easier to just enable the checks that were being implemented there than do manual verification that all instances of the issues were fixed. And if we feel like they're worth making a Nengo PR for, we might as well add those checks to Nengo Bones so that we don't add more docstrings that would fail those checks.

That said, this PR likely needs some discussion; see the todo section at the bottom.

How has this been tested?
Applied to Nengo Bones.

How long should this take to review?

  • Average (neither quick nor lengthy)

Types of changes:

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Still to do:

  • Come to consensus on what checks we want to add / ignore (most notably D401, see the squash commit in this PR)
  • Add documentation
  • Add changelog entry

D202,
D203,
D213,
D402,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably also want D401, which is the imperative mood one. Also, maybe we should put comments after each of these lines with a word or two saying what it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

The commit message of d2ce6d5 gives my thoughts on D401

Copy link
Member Author

@tbekolay tbekolay Aug 2, 2019

Choose a reason for hiding this comment

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

And I'd rather just link to http://www.pydocstyle.org/en/3.0.0/error_codes.html ... we don't give description for flake8, which is similarly code-based

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does take a bit of work to cross-reference the two lists, but not many people are going to be editing this, so it's not a big deal to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, D415 and D416 are missing from that list? They need to update their docs!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah they actually have a problem with their current docs right now as they don't render the table: PyCQA/pydocstyle#380

@hunse
Copy link
Collaborator

hunse commented Aug 2, 2019

For D301 (backslashes in docstring), the nengo.Connection docstring has a backslash for a line continuation, and it actually doesn't compile with Sphinx if you escape it (i.e. start with r"""). But, it does look bad when you call help on Connection because the unescaped backslash makes a ton of space.

@tbekolay
Copy link
Member Author

tbekolay commented Aug 2, 2019

For the Connection docstring, we managed to get around that because Black gives us 88 characters to work with instead of 79, so we didn't need the continuation character after all. For other cases of really long types, I think the general strategy should be to describe it more succinctly (there's almost always a way to reword).

@hunse
Copy link
Collaborator

hunse commented Aug 2, 2019

I also recall there being one place (with nested functions) where Black and pydocstyle were in conflict. pydocstyle wants no newline after function docstrings (D202). In this case, though, the function started with the definition of a helper function, and Black wanted a newline before that helper function definition. I can't seem to find where that was, though.

EDIT: Oh I see you've ignored D202. That works.

@tbekolay
Copy link
Member Author

tbekolay commented Aug 2, 2019

I ran these checks on nengo bones and nengo core and aside from D401 they pass, did you run it on any other projects?

@hunse
Copy link
Collaborator

hunse commented Aug 2, 2019

Nope, I haven't done it on others. I think those are all the tricky situations I found, and it looks like they're all resolved.

As for D401, my vote would be to try making the changes on Nengo and see how hard it is to do in practice, and whether it seems like an improvement.

@tbekolay
Copy link
Member Author

tbekolay commented Aug 2, 2019

Just a note that I marked this as a draft because the intention isn't to review/merge it now as I don't want to block nengo/nengo#1557 I just found this useful in reviewing nengo/nengo#1557 and made this so we can discuss it in the future (probably around the time of the Bones 1.0 work).

@drasmuss
Copy link
Member

drasmuss commented Aug 2, 2019

My vote would be to disable D401. I don't think it really helps with readability (we're all used to parsing sentences where the verb doesn't come first, or isn't imperative), and is just a pain in the nonzero number of cases where writing in non-imperative style is a natural way to document something. But I'm fine if someone wants to try it out 🤷‍♂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants