Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Optionally test for missing docstrings #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Optionally test for missing docstrings #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf3a0c1
Added functionality to check for missing docstrings on functions and …
krischer Jul 24, 2019
3187bee
Added test for the missing docstring check.
krischer Jul 24, 2019
dfe8ca8
Proper support for missing docstrings in the config file.
krischer Jul 24, 2019
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
18 changes: 14 additions & 4 deletions darglint/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,22 @@

class Configuration(object):

def __init__(self, ignore, message_template, style):
# type: (List[str], Optional[str], DocstringStyle) -> None
def __init__(self, ignore, message_template, style, raise_on_missing_docstrings):
# type: (List[str], Optional[str], DocstringStyle, bool) -> None
"""Initialize the configuration object.

Args:
ignore: A list of error codes to ignore.
message_template: the template with which to format the errors.
style: The style of docstring.
raise_on_missing_docstrings: Report an error if a public function or method
is missing a docstring.

"""
self.ignore = ignore
self.message_template = message_template
self.style = style
self.raise_on_missing_docstrings = raise_on_missing_docstrings


def load_config_file(filename): # type: (str) -> Configuration
Expand All @@ -64,6 +67,7 @@ def load_config_file(filename): # type: (str) -> Configuration
ignore = list()
message_template = None
style = DocstringStyle.GOOGLE
raise_on_missing_docstrings = False
if 'darglint' in config.sections():
if 'ignore' in config['darglint']:
errors = config['darglint']['ignore']
Expand All @@ -83,10 +87,15 @@ def load_config_file(filename): # type: (str) -> Configuration
[x.name for x in DocstringStyle]
)
)
if 'raise_on_missing_docstrings' in config['darglint']:
raise_on_missing_docstrings = config.getboolean(
'darglint', 'raise_on_missing_docstrings')

return Configuration(
ignore=ignore,
message_template=message_template,
style=style
style=style,
raise_on_missing_docstrings=raise_on_missing_docstrings
)


Expand Down Expand Up @@ -167,7 +176,8 @@ def get_config(): # type: () -> Configuration
return Configuration(
ignore=list(),
message_template=None,
style=DocstringStyle.GOOGLE
style=DocstringStyle.GOOGLE,
raise_on_missing_docstrings=False
)
return load_config_file(filename)

Expand Down
14 changes: 14 additions & 0 deletions darglint/driver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@
'only google or sphinx styles are supported.'
)
)
parser.add_argument(
'--raise-on-missing-docstrings',
action='store_true',
help=(
'Missing docstrings for public functions or methods are considered an '
'error.'
)
)

# ---------------------- MAIN SCRIPT ---------------------------------

Expand Down Expand Up @@ -138,6 +146,8 @@ def get_error_report(filename,

def print_error_list():
print('\n'.join([
'I002: Missing docstring for public method.'
'I003: Missing docstring for public function.'
'I101: The docstring is missing a parameter in the definition.',
'I102: The docstring contains a parameter not in function.',
'I103: The docstring parameter type doesn\'t match function.',
Expand Down Expand Up @@ -181,6 +191,10 @@ def main():
config.style = DocstringStyle.SPHINX
elif args.docstring_style == 'google':
config.style = DocstringStyle.GOOGLE

if args.raise_on_missing_docstrings:
config.raise_on_missing_docstrings = True

raise_errors_for_syntax = args.raise_syntax or False
for filename in args.files:
error_report = get_error_report(
Expand Down
45 changes: 45 additions & 0 deletions darglint/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
I Interface
S Style

000 Missing docstrings
100 Args
200 Returns
300 Yields
Expand Down Expand Up @@ -151,6 +152,50 @@ def __init__(self, function, message, line_numbers=None):
)


class MissingDocstringForPublicMethod(DarglintError):
"""Describes when a docstring is missing for a public method."""

error_code = 'I002'

def __init__(self, function, line_numbers=None):
# type: (Union[ast.FunctionDef, ast.AsyncFunctionDef], Tuple[int, int]) -> None
"""Instantiate the error's message.

Args:
function: An ast node for the function.
line_numbers: The line numbers where this error occurs.

"""
self.general_message = 'Missing docstring for public method'
self.terse_message = function.name
super(MissingDocstringForPublicMethod, self).__init__(
function,
line_numbers=line_numbers,
)


class MissingDocstringForPublicFunction(DarglintError):
"""Describes when a docstring is missing for a public function."""

error_code = 'I003'

def __init__(self, function, line_numbers=None):
# type: (Union[ast.FunctionDef, ast.AsyncFunctionDef], Tuple[int, int]) -> None
"""Instantiate the error's message.

Args:
function: An ast node for the function.
line_numbers: The line numbers where this error occurs.

"""
self.general_message = 'Missing docstring for public function'
self.terse_message = function.name
super(MissingDocstringForPublicFunction, self).__init__(
function,
line_numbers=line_numbers,
)


class MissingParameterError(DarglintError):
"""Describes when a docstring is missing a parameter in the definition."""

Expand Down
50 changes: 32 additions & 18 deletions darglint/integrity_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
ExcessReturnError,
ExcessVariableError,
ExcessYieldError,
MissingDocstringForPublicMethod,
MissingDocstringForPublicFunction,
MissingParameterError,
MissingRaiseError,
MissingReturnError,
Expand Down Expand Up @@ -64,7 +66,8 @@ def __init__(self,
config=Configuration(
ignore=[],
message_template=None,
style=DocstringStyle.GOOGLE
style=DocstringStyle.GOOGLE,
raise_on_missing_docstrings=False
),
raise_errors=False
):
Expand Down Expand Up @@ -93,8 +96,8 @@ def run_checks(self, function):

"""
self.function = function
if function.docstring is not None:
try:
try:
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move the try-block back inside of the conditional? The else-branch can't raise a ParserException, so there's no need for it to be included in the try-block.

if function.docstring is not None:
if self.config.style == DocstringStyle.GOOGLE:
self.docstring = Docstring.from_google(
function.docstring,
Expand All @@ -113,24 +116,35 @@ def run_checks(self, function):
self._check_yield()
self._check_raises()
self._sorted = False
except ParserException as exception:
# If a syntax exception was raised, we may still
# want to ignore it, if we place a noqa statement.
if (
SYNTAX_NOQA.search(function.docstring)
or EXPLICIT_GLOBAL_NOQA.search(function.docstring)
or BARE_NOQA.search(function.docstring)
):
return
if self.raise_errors:
raise
# If not docstring is given, and raise on missing is true, and its not
# a private function, add an error.
elif self.config.raise_on_missing_docstrings and not function.name.startswith("_"):
cls = (MissingDocstringForPublicMethod
Copy link
Owner

Choose a reason for hiding this comment

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

This would probably be more understandable if we moved the if-else to the top level, like

if function.is_method:
    self.errors.append(
        ...
    )
else:
    self.errors.append(
        ...
    )

That will make it more maintainable as well. (Say, if we decide we need to pass more information for functions, or if we change the __init__ signature for these twe errors.)

if function.is_method else MissingDocstringForPublicFunction)
self.errors.append(
exception.style_error(
cls(
self.function.function,
message=str(exception),
line_numbers=exception.line_numbers,
),
line_numbers=(-1, -1),
)
)
except ParserException as exception:
# If a syntax exception was raised, we may still
# want to ignore it, if we place a noqa statement.
if (
SYNTAX_NOQA.search(function.docstring)
or EXPLICIT_GLOBAL_NOQA.search(function.docstring)
or BARE_NOQA.search(function.docstring)
):
return
if self.raise_errors:
raise
self.errors.append(
exception.style_error(
self.function.function,
message=str(exception),
line_numbers=exception.line_numbers,
),
)

def _check_parameter_types(self):
# type: () -> None
Expand Down
82 changes: 81 additions & 1 deletion tests/test_integrity_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
MissingRaiseError,
MissingReturnError,
MissingYieldError,
MissingDocstringForPublicFunction,
MissingDocstringForPublicMethod,
ParameterTypeMismatchError,
ReturnTypeMismatchError,
)
Expand All @@ -30,7 +32,8 @@ def setUp(self):
self.config = Configuration(
ignore=[],
message_template=None,
style=DocstringStyle.SPHINX
style=DocstringStyle.SPHINX,
raise_on_missing_docstrings=False
)

def test_missing_parameter(self):
Expand Down Expand Up @@ -644,3 +647,80 @@ def test_global_noqa_works_for_syntax_errors(self):
])
for variant in ['# noqa: *', '# noqa: S001', '# noqa']:
self.has_no_errors(program.format(variant))

self.config = Configuration(
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this, or comment on its purpose. (It's a little confusing here.)

ignore=[],
message_template=None,
style=DocstringStyle.SPHINX,
raise_on_missing_docstrings=False
)

def test_check_for_missing_docstrings(self):
Copy link
Owner

Choose a reason for hiding this comment

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

This could probably be broken up into multiple unit tests. If it's in one unit tests because of the hassle of performing configuration, etc., then you could put it into its own TestCase and have a setUp method. If it's broken up, it will make failing tests easier to diagnose. If we somehow broke the check for missing docstrings on functions but not methods, for example, then the method's name would immediately let us know what is wrong.

"""Make sure we capture missing parameters."""
program = '\n'.join([
# Function without a docstring.
'def foo():',
' pass',
# Function with a docstring.
'def bar():',
' """"docstring"""',
' pass',
# private function without a docstring.
'def _foo():',
' pass',
'class FooBar:',
# Method with a docstring.
' def bar_method(self):',
' """docstring"""',
' pass',
# Method without a docstring.
' def foo_method(self):',
' pass',
# Private method without a docstring.
' def _foo_method(self):',
' pass',
])
tree = ast.parse(program)
functions = get_function_descriptions(tree)

# No errors if raise_on_missing_docstrings is False.
checker = IntegrityChecker(
Configuration(
ignore=[],
message_template=None,
style=DocstringStyle.GOOGLE,
raise_on_missing_docstrings=False
)
)
for f in functions:
checker.run_checks(f)
errors = checker.errors
self.assertEqual(len(errors), 0)

# But two errors if True.
checker = IntegrityChecker(
Configuration(
ignore=[],
message_template=None,
style=DocstringStyle.GOOGLE,
raise_on_missing_docstrings=True
)
)
for f in functions:
checker.run_checks(f)
errors = checker.errors
self.assertEqual(len(errors), 2)

# Somehow the functions are parsed methods -> functions.
self.assertTrue(isinstance(
errors[0], MissingDocstringForPublicMethod))
self.assertTrue(isinstance(
errors[1], MissingDocstringForPublicFunction))
self.assertEqual(errors[0].general_message,
'Missing docstring for public method')
self.assertEqual(errors[0].terse_message,
'foo_method')
self.assertEqual(errors[1].general_message,
'Missing docstring for public function')
self.assertEqual(errors[1].terse_message,
'foo')