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

Optional checks for multi-line docstring summary style. #174

Merged
merged 1 commit into from Mar 25, 2016

Conversation

Projects
None yet
3 participants
@toroettg
Copy link
Contributor

toroettg commented Feb 20, 2016

Hello,

PEP 257 allows multi-line docstring summaries to start on the first or second line, as it is outlined in #44. I prefer one over the other and was looking for a way to perform consistency checks on my projects. Thus, I have added two optional error codes (D212, D213), which you might like to incorporate.

As those are more restrictive than PEP 257, both error codes should naturally be selected explicitly (and typically mutual exclusive) by the following. Otherwise, they are ignored.

pydocstyle --add-select=D212,D213 file_to_check.py

I have tried to touch as little as possible when implementing them. Please review if this is too cumbersome, or interferes with pydocstyle in a way I have not noticed.

Kind regards,
Tobias

@toroettg toroettg changed the title Added optional error codes D212 and D213. Optional checks for multi-line docstring summary style. Feb 20, 2016

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Feb 21, 2016

Thanks for submitting this! The concept is good, and I'll try to review the code as soon as I can.


@expect('D212: Multi-line docstring summary does not start on the first line.')
def multi_line_starts_second_line():
"""

This comment has been minimized.

@jayvdb

jayvdb Feb 22, 2016

Member

could you add some tests that use u"""...""" , r"""...""" ,etc. I suspect that if first not in ['"""', "'''"]: wont catch them all.

@jayvdb

This comment has been minimized.

Copy link
Member

jayvdb commented Feb 22, 2016

Fwiw, it was on my todo list; my main project wants to stablise our docstrings according to your D212 rule ;-) So I am thrilled to see this patch.

When you get a chance, please squash your two commits into a single commit. Thanks.

Just as an aside, I believe D213 equals H404. https://github.com/openstack-dev/hacking/blob/master/hacking/checks/docstrings.py#L77

@toroettg toroettg force-pushed the toroettg:MultiLineStyleOption branch from e952a9f to 973d8d1 Feb 22, 2016

@toroettg

This comment has been minimized.

Copy link
Contributor

toroettg commented Feb 22, 2016

@jayvdb

You are right, I've added additional checks, but they quickly grow tedious. Another
abstraction layer like @check_for(Content), to test only literals between docstring opening and closing, would come in handy to reduce redundancy.

Another enhancement for the test framework, which came to my mind, was to test against virtual files, whose content is generated dynamically at run time from definitions in test cases.

H404 seems to be equal to D212; I have somewhat harmonized this PR with their code, since I am reinventing the wheel already.

Kind regards,
Tobias

@jayvdb

This comment has been minimized.

Copy link
Member

jayvdb commented Feb 22, 2016

Another abstraction layer like @check_for(Content),
to test only literals between docstring opening and closing,
would come in handy to reduce redundancy.

That would be very nice. It would be a separate changeset.

@@ -14,4 +14,4 @@ Not all error codes are checked for by default. The default behavior is to
check only error codes that are part of the `PEP257
<http://www.python.org/dev/peps/pep-0257/>`_ official convention.

All of the above error codes are checked for by default except for D203.
All of the above error codes are checked for by default except for D203, D212 and D213.

This comment has been minimized.

@Nurdok

Nurdok Feb 23, 2016

Member

Please make sure you don't exceed 80 columns per line.

D212 = D2xx.create_error('D212', 'Multi-line docstring summary does not start '
'on the first line.')
D213 = D2xx.create_error('D213', 'Multi-line docstring summary does not start '
'on a separate line.')

This comment has been minimized.

@Nurdok

Nurdok Feb 23, 2016

Member

I prefer a positive phrasing for these - "Multi-line docstring summary should start on the first line" and "Multi-line docstring should start on a separate line".

(test_cases, set(ErrorRegistry.get_error_codes()) - optional_codes),
(option_test_cases, optional_codes)
)

This comment has been minimized.

@Nurdok

Nurdok Feb 23, 2016

Member

Instead of all of this, you can just configure the test cases to always run on all the errors. It's okay if we'll get mutually exclusive errors in these and it will be less of a hassle.

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Feb 23, 2016

Make sure to also add your change to the changelog.

@toroettg toroettg force-pushed the toroettg:MultiLineStyleOption branch from 973d8d1 to 53ff113 Mar 12, 2016

@toroettg

This comment has been minimized.

Copy link
Contributor

toroettg commented Mar 12, 2016

I have adapted the PR to your requested changes. With the integration as a regular definition test, I had to remove the Unicode related test cases, since they are invalid in the current PyPy3 (v2.4.0). Those may be added to #178 if so desired.

Also, the expect decorator gets confused by required arguments. Please review the change at a_following_valid_function for any side effects.

@Nurdok Nurdok merged commit c3cd2dc into PyCQA:master Mar 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Mar 25, 2016

@toroettg Merged! Thank you so much for your time and effort, and sorry for taking so long to merge this.

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