Skip to content
This repository was archived by the owner on Nov 3, 2023. It is now read-only.

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

Merged
merged 6 commits into from
Aug 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/release_notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ Release Notes
**pydocstyle** version numbers follow the
`Semantic Versioning <http://semver.org/>`_ specification.

Current Development Version
---------------------------

Bug Fixes

* D401: Fixed a false positive where one stem had multiple imperative forms,
e.g., init and initialize / initiate (#382).

4.0.0 - July 6th, 2019
---------------------------

Expand Down
12 changes: 8 additions & 4 deletions src/pydocstyle/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from .parser import (Package, Module, Class, NestedClass, Definition, AllError,
Method, Function, NestedFunction, Parser, StringIO,
ParseError)
from .utils import log, is_blank, pairwise
from .utils import log, is_blank, pairwise, common_prefix_length
from .wordlists import IMPERATIVE_VERBS, IMPERATIVE_BLACKLIST, stem


Expand Down Expand Up @@ -440,16 +440,20 @@ def check_imperative_mood(self, function, docstring): # def context
return violations.D401b(first_word)

try:
correct_form = IMPERATIVE_VERBS.get(stem(check_word))
correct_forms = IMPERATIVE_VERBS.get(stem(check_word))
except UnicodeDecodeError:
# This is raised when the docstring contains unicode
# characters in the first word, but is not a unicode
# string. In which case D302 will be reported. Ignoring.
return

if correct_form and correct_form != check_word:
if correct_forms and check_word not in correct_forms:
best = max(
correct_forms,
key=lambda f: common_prefix_length(check_word, f)
)
return violations.D401(
correct_form.capitalize(),
best.capitalize(),
first_word
)

Expand Down
13 changes: 13 additions & 0 deletions src/pydocstyle/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,16 @@ def pairwise(
a, b = tee(iterable)
_ = next(b, default_value)
return zip_longest(a, b, fillvalue=default_value)


def common_prefix_length(a: str, b: str) -> int:
"""Return the length of the longest common prefix of a and b.

>>> common_prefix_length('abcd', 'abce')
3

"""
for common, (ca, cb) in enumerate(zip(a, b)):
if ca != cb:
return common
return min(len(a), len(b))
4 changes: 2 additions & 2 deletions src/pydocstyle/violations.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def create_error(
self,
error_code: str,
error_desc: str,
error_context: Optional[str]=None,
error_context: Optional[str] = None,
) -> Callable[[Iterable[str]], Error]:
"""Create an error, register it to this group and return it."""
# TODO: check prefix
Expand Down Expand Up @@ -219,7 +219,7 @@ def to_rst(cls) -> str:
D400 = D4xx.create_error('D400', 'First line should end with a period',
'not {0!r}')
D401 = D4xx.create_error('D401', 'First line should be in imperative mood',
"'{0}', not '{1}'")
"perhaps '{0}', not '{1}'")
D401b = D4xx.create_error('D401', 'First line should be in imperative mood; '
'try rephrasing', "found '{0}'")
D402 = D4xx.create_error('D402', 'First line should not be the function\'s '
Expand Down
11 changes: 9 additions & 2 deletions src/pydocstyle/wordlists.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import re
import pkgutil
import snowballstemmer
from typing import Iterator
from typing import Iterator, Dict, Set


#: Regular expression for stripping comments from the wordlists
Expand Down Expand Up @@ -36,7 +36,14 @@ def load_wordlist(name: str) -> Iterator[str]:


#: A dict mapping stemmed verbs to the imperative form
IMPERATIVE_VERBS = {stem(v): v for v in load_wordlist('imperatives.txt')}
def make_imperative_verbs_dict(wordlist: Iterator[str]) -> Dict[str, Set[str]]:
imperative_verbs = {} # type: Dict[str, Set[str]]
for word in wordlist:
imperative_verbs.setdefault(stem(word), set()).add(word)
return imperative_verbs


IMPERATIVE_VERBS = make_imperative_verbs_dict(load_wordlist('imperatives.txt'))

#: Words that are forbidden to appear as the first word in a docstring
IMPERATIVE_BLACKLIST = set(load_wordlist('imperatives_blacklist.txt'))
3 changes: 2 additions & 1 deletion src/tests/test_cases/noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')")
Copy link
Contributor

Choose a reason for hiding this comment

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

"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"?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

def docstring_ignore_some_violations_but_catch_D401(): # noqa: E501,D400,D415
"""Runs something"""
pass
2 changes: 1 addition & 1 deletion src/tests/test_cases/sections.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def ignore_non_actual_section(): # noqa: D416

@expect(_D213)
@expect("D401: First line should be in imperative mood "
"('Return', not 'Returns')")
"(perhaps 'Return', not 'Returns')")
@expect("D400: First line should end with a period (not 's')")
@expect("D415: First line should end with a period, question "
"mark, or exclamation point (not 's')")
Expand Down
26 changes: 22 additions & 4 deletions src/tests/test_cases/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ def lwnlkjl():
"""Summary"""


@expect("D401: First line should be in imperative mood ('Return', not "
"'Returns')")
@expect("D401: First line should be in imperative mood "
"(perhaps 'Return', not 'Returns')")
def liouiwnlkjl():
"""Returns foo."""

Expand Down Expand Up @@ -361,7 +361,8 @@ def inner_function():


@expect("D400: First line should end with a period (not 'g')")
@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')")
@expect("D415: First line should end with a period, question mark, "
"or exclamation point (not 'g')")
def docstring_bad():
Expand All @@ -379,12 +380,29 @@ 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')")
def docstring_ignore_some_violations_but_catch_D401(): # noqa: E501,D400,D415
"""Runs something"""
pass


@expect(
"D401: First line should be in imperative mood "
"(perhaps 'Initiate', not 'Initiates')"
)
def docstring_initiates():
"""Initiates the process."""


@expect(
"D401: First line should be in imperative mood "
"(perhaps 'Initialize', not 'Initializes')"
)
def docstring_initializes():
"""Initializes the process."""


@wraps(docstring_bad_ignore_one)
def bad_decorated_function():
"""Bad (E501) but decorated"""
Expand Down
28 changes: 28 additions & 0 deletions src/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Unit test for pydocstyle utils.

Use tox or py.test to run the test suite.
"""
from pydocstyle import utils


__all__ = ()


def test_common_prefix():
"""Test common prefix length of two strings."""
assert utils.common_prefix_length('abcd', 'abce') == 3


def test_no_common_prefix():
"""Test common prefix length of two strings that have no common prefix."""
assert utils.common_prefix_length('abcd', 'cdef') == 0


def test_differ_length():
"""Test common prefix length of two strings differing in length."""
assert utils.common_prefix_length('abcd', 'ab') == 2


def test_empty_string():
"""Test common prefix length of two strings, one of them empty."""
assert utils.common_prefix_length('abcd', '') == 0