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 an initial Google convention #357

Merged
merged 4 commits into from May 26, 2019
Merged

Add an initial Google convention #357

merged 4 commits into from May 26, 2019

Conversation

@samj1912
Copy link
Contributor

samj1912 commented Apr 19, 2019

Sources for docstring convention:

Google Style Guide and Napoleon Google Style Guide

Rationale for exclusions of errors:

  • D203: Multiple examples with class docstrings without blank line.
  • D204: Same as above.
  • D213: All multiline summaries start immediately after the triple quotes.
  • D215: N/A no underlines.
  • D400: First line can end with "period, question mark, or exclamation
    point".
  • D401: Style guide explicitly says not to use imperative mood and use
    descriptive mood.
  • D404: Clearly allowed WRT to the above. There is also an example using
    "This".
  • D406: Section names end with a colon.
  • D407: N/A no underlines.
  • D408: N/A no underlines.
  • D409: N/A no underlines.

Adds the following violations:

  • D415: First line of docstring should end with a period, question mark, or exclamation point
  • D416: Section name should end with a semicolon
  • D417: Missing arguments in the function docstring

Thanks for submitting a PR!

Please make sure to check for the following items:

  • Add unit tests and integration tests where applicable.
    If you've added an error code or changed an error code behavior,
    you should probably add or change a test case file under tests/test_cases/ and add
    it to the list under tests/test_definitions.py.
    If you've added or changed a command line option,
    you should probably add or change a test in tests/test_integration.py.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

Please don't get discouraged as it may take a while to get a review.

Sources for docstring convention:
[Google Style Guide](http://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings)
and [Napoleon Google Style Guide](https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html)

Rationale for exclusions of errors:
D203: Multiple examples with class docstrings without blank line.
D204: Same as above.
D213: All multiline summaries start immediately after the triple quotes.
D215: N/A no underlines.
D400: First line can end with "period, question mark, or exclamation
point".
D401: Style guide explicitly says not to use imperative mood and use
descriptive mood.
D404: Clearly allowed WRT to the above. There is also an example using
"This".
D406: Section names end with a colon.
D407: N/A no underlines.
D408: N/A no underlines.
D409: N/A no underlines.
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 20, 2019

TODO for initial PR:

  • Add google style args check
  • Add google style section check
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 20, 2019

While writing this patch I am coming to the realization that conventions, as they are currently defined in pydocstyle, might lead to too complex code. Rather than having them as a list of errors to ignore it makes more sense for each of them to define a certain set of errors instead, following which the checkers should be segregated to match the conventions.

But this might be a backwards incompatible change especially since ignores and conventions are mutually exclusive.

Can any of the maintainers(@Nurdok , @shacharoo ) of the repo chip-in here with the best way forward?

I will meanwhile continue to write the checkers given the current constraints of conventions and ignores.

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Apr 20, 2019

I'm not a maintainer, but I'm wondering if style conventions might be better if they were treated like plugins such that new styles (like this one) can be distributed separately from pydocstyle

@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 20, 2019

I am not sure a plugin system exists for pydocstyle, but yeah, if it did, this PR would've been better as a plugin.

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Apr 20, 2019

I am not sure a plugin system exists for pydocstyle,

I took a quick look and I think the answer is no

@samj1912 samj1912 force-pushed the samj1912:google-style branch from bf9f331 to 56e0eb8 Apr 21, 2019
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 21, 2019

I have added google style sections. I guess this PR is big enough as it is. For further section level checks
(like paramter checks in Args/Attributes sections etc.) I will be creating a separate PR.

@samj1912 samj1912 marked this pull request as ready for review Apr 21, 2019
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 21, 2019

Once this is reviewed, I plan on submitting another PR refactoring the ConventionChecker into more modular parts and maybe we can discuss how to handle these styles in general.

But this PR should be good to go as it is.

@samj1912 samj1912 force-pushed the samj1912:google-style branch 2 times, most recently from 6c056a7 to bc25cb8 Apr 21, 2019
@samj1912 samj1912 force-pushed the samj1912:google-style branch 2 times, most recently from 08b3ce1 to 0992ca5 Apr 21, 2019
@shacharoo

This comment has been minimized.

Copy link
Member

shacharoo commented Apr 21, 2019

@sigmavirus24 I think Pydocstyle shouldn't become a plug-and-play system for running convention checkers. Code should comply with the conventions we check in it, not manipulate it to fit their specific needs.

@samj1912 - I'll try to make some time this weekend to take a look at it. Thanks for contributing!

@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 21, 2019

@shacharoo thanks :) You will find that all the commits are self-contained. So in case the PR is too big to review, I can chop the PR up by commits easily. (Or if you want you can review it commit by commit 😃 )

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Apr 21, 2019

@shacharoo I'm not advocating auto-fixing, I'm advocating the --convention flag become extendable

Copy link
Member

Nurdok left a comment

Thanks for the contribution! I've skimmed a bit to give you some initial feedback - I'll try to take a closer look in the weekend as well.

src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/tests/test_cases/test.py Outdated Show resolved Hide resolved
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Apr 22, 2019

I'm not advocating auto-fixing, I'm advocating the --convention flag become extendable

Why? I think PEP-257, numpy and the Google convention are popular enough and have a big enough traction in their own fields to merit enforcement. But I definitely don't want to encourage folks to create their own conventions. If I could eliminate all but one convention, I would :)

Pydocstyle has the option to enable and disable violations to support a gradual ramp up to full conformance and I accept that some codebases will never fully conform (and some conformance may be better than none). But I'm on the side of having an opinionated linter - that only helps you if you're trying to stick to a known convention. If some developers somewhere want to support a new convention other than the ones mentioned here, I sincerely hope they fail and adopt PEP-257 instead 😄

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Apr 22, 2019

If I could eliminate all but one convention, I would :)

What if I told you, by allowing plug-able conventions, you could. ;)

If some developers somewhere want to support a new convention other than the ones mentioned here, I sincerely hope they fail and adopt PEP-257 instead

There are code-bases much older than PEP-257 and older than this tool. Those code-bases probably have conventions they'd like to enforce but have to do manually in a completely error-prone way and updating them is toilsome and a potentially ridiculous amount of work. I, for one, really don't think that they should have to fork this tool in order to gain the benefits of automated docstring style enforcement.

Only having 1 (or 2, or 3, ... or 5) standards works well enough when it's considered very early on (see also golint, rust-lang's doc format, etc.) but for Python we don't have that luxury and saying "I'll only support these hand-waving number of conventions because they're the most popular" comes across really poorly to the rest of the Python world.

@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 22, 2019

FWIW I implemented a plugin system in 4d4c544

The PR is at #359 (although it says 800 loc most of the code is from #358 ) and the actual plugin system after the refactor is 30 loc.

Obviously it is quite rudimentary but works as intended.

@samj1912 samj1912 force-pushed the samj1912:google-style branch from c028849 to 324a488 Apr 28, 2019
This commit adds error code D415 which allows for Google
style end char checking (".", "?", "!").
@samj1912 samj1912 force-pushed the samj1912:google-style branch from 324a488 to f7f3443 Apr 28, 2019
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 28, 2019

@Nurdok @shacharoo all existing comments have been resolved.

@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented Apr 28, 2019

Also, I added some missing support for D414 to both numpy and google style sections.

@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented May 12, 2019

src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
@samj1912 samj1912 force-pushed the samj1912:google-style branch from f7f3443 to 82acdd6 May 19, 2019
samj1912 added 2 commits Apr 21, 2019
pydocstyle now supports checking for missing function arguments
from docstrings.
@samj1912 samj1912 force-pushed the samj1912:google-style branch from 82acdd6 to 2f25e0e May 19, 2019
@samj1912

This comment has been minimized.

Copy link
Contributor Author

samj1912 commented May 25, 2019

@Nurdok @shacharoo any chance you can take a look at this, this weekend?

@Nurdok
Nurdok approved these changes May 26, 2019
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented May 26, 2019

@samj1912 Thank you so much for doing this. Really sorry for the long delays (@shacharoo just got married and I was abroad several times, to our defense 😄). Merging.

@Nurdok Nurdok merged commit 6de6d93 into PyCQA:master May 26, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mkhorton

This comment has been minimized.

Copy link

mkhorton commented Oct 15, 2019

Just a quick note that your usage doc hasn't been updated with this new option, found this PR by accident. Thanks for adding support for google conventions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.