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

D401: Allow multiple imperative forms of the same stem. #382

Merged
merged 6 commits into from Aug 2, 2019
Merged

Conversation

@Nurdok
Copy link
Member

Nurdok commented Jul 20, 2019

  • Added tests.
  • Added a line to the release notes.
@Nurdok Nurdok requested review from lordmauve and shacharoo Jul 20, 2019
@Nurdok Nurdok marked this pull request as ready for review Jul 20, 2019
@Nurdok Nurdok self-assigned this Jul 20, 2019
@Nurdok Nurdok added the Bugfix label Jul 20, 2019
Copy link
Member

shacharoo left a comment

image

Copy link
Contributor

lordmauve left a comment

Thanks for this. I spotted some issues in my own code when you reviewed it back to me. Want me to fix them?

src/tests/test_utils.py Outdated Show resolved Hide resolved
@@ -16,7 +16,8 @@ def docstring_bad_ignore_one(): # noqa: D400,D401,D415
pass


@expect("D401: First line should be in imperative mood ('Run', not 'Runs')")
@expect("D401: First line should be in imperative mood "
"(perhaps 'Run', not 'Runs')")

This comment has been minimized.

Copy link
@lordmauve

lordmauve Jul 22, 2019

Contributor

"perhaps y, not x" reads clumsily to me. Adding "perhaps y" suggests a lot less confidence in the recommendation, while "not x" seems very confident.

Maybe "eg. y instead of x"?

This comment has been minimized.

Copy link
@Nurdok

Nurdok Aug 2, 2019

Author Member

Seeing as the bug was caused by two different imperative forms of the same stem, I was indeed going for a less confident recommendation.

Some options here:

  1. perhaps 'Run', not 'Runs'
  2. 'Run', not 'Runs'
  3. e.g., 'Run' instead of 'Runs'
  4. perhaps 'Run' instead of 'Runs'

I chose (1) because it's more clear that 'Runs' is definitely bad while 'Run' is not necessarily the right choice.

@shacharoo LMKWYT

This comment has been minimized.

Copy link
@Nurdok

Nurdok Aug 2, 2019

Author Member

I'm merging so that other PRs are unblocked, I think the current phrasing is fine for now, but we can revisit it before the next release.

This comment has been minimized.

Copy link
@lordmauve

lordmauve Aug 3, 2019

Contributor

Thanks. (I like 3 and 4 for what it's worth).

src/pydocstyle/utils.py Outdated Show resolved Hide resolved
@Nurdok Nurdok merged commit 0a2f3ef into master Aug 2, 2019
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
lordmauve added a commit to lordmauve/pydocstyle that referenced this pull request Nov 20, 2019
* Allow multiple imperative forms of the same stem.

* Update phrasing of violation.

* Select best imperative by longest prefix match

* Fixed test after merge.

* Added line to release notes.

* Code review fixes.
lordmauve added a commit to lordmauve/pydocstyle that referenced this pull request Nov 20, 2019
* Allow multiple imperative forms of the same stem.

* Update phrasing of violation.

* Select best imperative by longest prefix match

* Fixed test after merge.

* Added line to release notes.

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