-
Notifications
You must be signed in to change notification settings - Fork 187
Introduce convention class #589
Changes from all commits
801fc92
6d3a59d
9a54a2a
8e9ef40
0a393a2
0c9ba5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
"""This module contains the convention definitions.""" | ||
|
||
from typing import Set | ||
|
||
from pydocstyle.violations import all_errors | ||
|
||
CONVENTION_NAMES = ("pep257", "numpy", "google") | ||
|
||
|
||
convention_errors = { | ||
'pep257': all_errors | ||
- { | ||
'D203', | ||
'D212', | ||
'D213', | ||
'D214', | ||
'D215', | ||
'D404', | ||
'D405', | ||
'D406', | ||
'D407', | ||
'D408', | ||
'D409', | ||
'D410', | ||
'D411', | ||
'D413', | ||
'D415', | ||
'D416', | ||
'D417', | ||
'D418', | ||
}, | ||
'numpy': all_errors | ||
- { | ||
'D107', | ||
'D203', | ||
'D212', | ||
'D213', | ||
'D402', | ||
'D413', | ||
'D415', | ||
'D416', | ||
'D417', | ||
}, | ||
'google': all_errors | ||
- { | ||
'D203', | ||
'D204', | ||
'D213', | ||
'D215', | ||
'D400', | ||
'D401', | ||
'D404', | ||
'D406', | ||
'D407', | ||
'D408', | ||
'D409', | ||
'D413', | ||
}, | ||
} | ||
|
||
|
||
class Convention: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the advantage of using a class instead of just using an Enum and keeping the AttrDict? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My idea was to eventually have the convention class itself check the code and select the appropriate checker function. You can probably also just throw the name of the convention into the convention checker and that's it. |
||
"""This class defines the convention to use for checking docstrings.""" | ||
|
||
def __init__(self, name: str = "pep257") -> None: | ||
"""Initialize the convention. | ||
|
||
The convention has two purposes. First, it holds the error codes to be | ||
checked. Second, it defines how to treat docstrings, eliminating the | ||
need for extra logic to determine whether a docstring is a NumPy or | ||
Google style docstring. | ||
Comment on lines
+68
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second part seems especially useful to me. Going very far down the road to DWIM ("do what I mean") gets pretty painful. Users end up having to trick the tool into doing what they want instead of just specifying it as this approach allows. |
||
|
||
Each convention has a set of error codes to check as a baseline. | ||
Specific error codes can be added or removed via the | ||
:code:`add_error_codes` or :codes:`remove_error_codes` methods. | ||
|
||
Args: | ||
name: The convention to use. Defaults to "pep257". | ||
|
||
Raises: | ||
ValueError: If an unsupported convention is specified. | ||
""" | ||
if name not in CONVENTION_NAMES: | ||
raise ValueError(f"Unknown convention: '{name}'") | ||
|
||
self.name = name | ||
self.error_codes = convention_errors[name] | ||
|
||
def add_error_codes(self, error_codes: Set[str]) -> None: | ||
"""Add additional error codes to the convention. | ||
|
||
Args: | ||
error_codes: The error codes to also check. | ||
""" | ||
self.error_codes = self.error_codes.union(error_codes) | ||
|
||
def remove_error_codes(self, error_codes: Set[str]) -> None: | ||
Comment on lines
+89
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you instead define convention = Convention()
convention += {"D103"}
convention -= {"D101", "D102"} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's possible, but at least to me it makes the whole thing less obvious, without offering a real benefit. |
||
"""Remove error codes from the convention. | ||
|
||
Args: | ||
error_codes: The error codes to ignore. | ||
""" | ||
self.error_codes = self.error_codes - error_codes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
"""This module contains tests for the available conventions.""" | ||
|
||
import re | ||
|
||
import pytest | ||
|
||
from pydocstyle.conventions import CONVENTION_NAMES, Convention | ||
|
||
|
||
def test_default_convention_is_pep257() -> None: | ||
"""Test that pep257 is used as the default convention.""" | ||
convention = Convention() | ||
|
||
assert convention.name == "pep257" | ||
|
||
|
||
def test_invalid_convention_raises_error() -> None: | ||
"""Test that using an invalid convention name raises an error.""" | ||
with pytest.raises( | ||
ValueError, match="Unknown convention: 'invalid_convention'" | ||
): | ||
Convention("invalid_convention") | ||
|
||
|
||
@pytest.mark.parametrize("convention_name", CONVENTION_NAMES) | ||
def test_names_are_set_correctly(convention_name: str) -> None: | ||
"""Test that the convention holds its name as an attribute.""" | ||
convention = Convention(convention_name) | ||
|
||
assert convention.name == convention_name | ||
|
||
|
||
@pytest.mark.parametrize("convention_name", CONVENTION_NAMES) | ||
def test_conventions_keep_their_error_codes_as_attribute( | ||
convention_name: str, | ||
) -> None: | ||
"""Check that conventions are initialized with a set of error codes.""" | ||
convention = Convention(convention_name) | ||
|
||
assert len(convention.error_codes) > 0 | ||
|
||
for error_code in convention.error_codes: | ||
assert len(error_code) == 4 | ||
assert re.compile(r"D[1-4]\d\d").match(error_code) | ||
|
||
|
||
def test_can_add_error_codes() -> None: | ||
"""Test that additional error codes can be added to a convention.""" | ||
convention = Convention() | ||
|
||
n_errors_before_adding = len(convention.error_codes) | ||
|
||
assert "D203" not in convention.error_codes | ||
assert "D212" not in convention.error_codes | ||
|
||
convention.add_error_codes({"D203", "D212"}) | ||
|
||
assert len(convention.error_codes) - n_errors_before_adding == 2 | ||
|
||
assert "D203" in convention.error_codes | ||
assert "D212" in convention.error_codes | ||
|
||
|
||
def test_can_remove_error_codes() -> None: | ||
"""Test that specific error codes can be removed from a convention.""" | ||
convention = Convention() | ||
|
||
n_errors_before_removal = len(convention.error_codes) | ||
|
||
assert "D100" in convention.error_codes | ||
assert "D102" in convention.error_codes | ||
|
||
convention.remove_error_codes({"D100", "D102"}) | ||
|
||
assert n_errors_before_removal - len(convention.error_codes) == 2 | ||
|
||
assert "D100" not in convention.error_codes | ||
assert "D102" not in convention.error_codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there is a common subset of the errors being removed from each of the conventions that makes sense to name. E.g., something along the lines of "very strict rules".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't love the "all_errors - {subset}" logic, but not just because of code duplication. Any time anyone adds a new convention, we have to add it to this list. Personally, it makes more sense to me to make these additive instead of subtractive. But let's save this for another issue/PR since this PR doesn't change anything, just moves it around.