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

Bugfix: Allow comments before module docstring noqa codes #446

Merged
merged 10 commits into from Jul 15, 2020
2 changes: 2 additions & 0 deletions docs/release_notes.rst
Expand Up @@ -17,6 +17,8 @@ Bug Fixes
* Detect inner asynchronous functions for D202 (#467)
* Fix a bug in parsing Google-style argument description.
The bug caused some argument names to go unreported in D417 (#448).
* Fixed an issue where skipping errors on module level docstring via #noqa
failed when there where more prior comments (#446).

5.0.2 - January 8th, 2020
---------------------------
Expand Down
20 changes: 14 additions & 6 deletions src/pydocstyle/parser.py
Expand Up @@ -584,12 +584,20 @@ def parse_definition(self, class_):
def parse_skip_comment(self):
"""Parse a definition comment for noqa skips."""
skipped_error_codes = ''
if self.current.kind == tk.COMMENT:
if 'noqa: ' in self.current.value:
skipped_error_codes = ''.join(
self.current.value.split('noqa: ')[1:])
elif self.current.value.startswith('# noqa'):
skipped_error_codes = 'all'
while self.current.kind in (tk.COMMENT, tk.NEWLINE, tk.NL):
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a file only has comments and the stream gets exhausted? Will the current.kind access lead to Type Error because of self.current being None? Can we add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried all kinds of empty files, files with only comments etc. Nothing unexpected occurs. My understanding is that if the file has only comments, this code won't be called anyway.

As for adding a test for a file with all comments, I don't see that fitting in with the current testing setup. This is because the test case files are expected to have an expectation variable defined. Can't do that in an empty file. If you want such a test, how would you go about implementing it?

Copy link
Member

Choose a reason for hiding this comment

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

You can add an integration test https://github.com/PyCQA/pydocstyle/blob/master/src/tests/test_integration.py#L333

See above for example. You can add tests with an empty file and check that the output code is 0

Copy link
Contributor

@Cielquan Cielquan Jun 4, 2020

Choose a reason for hiding this comment

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

I added two integration tests:

  1. Test that a file with only comments does only cause a D100.
  2. Test that a file with comments and docstring does not throw an error.

@samj1912 Is this ok?

EDIT: Added two more tests but with # noaq comments which tests the fixed behavior stated in the opening post.

if self.current.kind == tk.COMMENT:
if 'noqa: ' in self.current.value:
skipped_error_codes = ''.join(
bsamseth marked this conversation as resolved.
Show resolved Hide resolved
self.current.value.split('noqa: ')[1:])
elif self.current.value.startswith('# noqa'):
skipped_error_codes = 'all'
self.stream.move()
self.log.debug("parsing comments before docstring, token is %r (%s)",
self.current.kind, self.current.value)

if skipped_error_codes:
break

return skipped_error_codes

def check_current(self, kind=None, value=None):
Expand Down
1 change: 1 addition & 0 deletions src/tests/test_cases/noqa.py
@@ -1,3 +1,4 @@
# -*- coding: utf-8 -*-
# noqa: D400,D415
"""Test case for "# noqa" comments"""
from .expected import Expectation
Expand Down
67 changes: 67 additions & 0 deletions src/tests/test_integration.py
Expand Up @@ -1123,3 +1123,70 @@ def bar(a):
out, err, code = env.invoke(args="-v")
assert code == 0
assert "IndentationError: unexpected indent" not in err


def test_only_comment_file(env):
"""Test that file with only comments does only cause D100."""
with env.open('comments.py', 'wt') as comments:
comments.write(
'#!/usr/bin/env python3\n'
'# -*- coding: utf-8 -*-\n'
'# Useless comment\n'
'# Just another useless comment\n'
)

out, _, code = env.invoke()
assert 'D100' in out
out = out.replace('D100', '')
for err in {'D1', 'D2', 'D3', 'D4'}:
assert err not in out
assert code == 1


def test_comment_plus_docstring_file(env):
"""Test that file with comments and docstring does not cause errors."""
with env.open('comments_plus.py', 'wt') as comments_plus:
comments_plus.write(
'#!/usr/bin/env python3\n'
'# -*- coding: utf-8 -*-\n'
'# Useless comment\n'
'# Just another useless comment\n'
'"""Module docstring."""\n'
)

out, _, code = env.invoke()
assert '' == out
assert code == 0


def test_only_comment_with_noqa_file(env):
"""Test that file with noqa and only comments does not cause errors."""
with env.open('comments.py', 'wt') as comments:
comments.write(
'#!/usr/bin/env python3\n'
'# -*- coding: utf-8 -*-\n'
'# Useless comment\n'
'# Just another useless comment\n'
'# noqa: D100\n'
)

out, _, code = env.invoke()
assert 'D100' not in out
assert code == 0


def test_comment_with_noqa_plus_docstring_file(env):
"""Test that file with comments, noqa, docstring does not cause errors."""
with env.open('comments_plus.py', 'wt') as comments_plus:
comments_plus.write(
'#!/usr/bin/env python3\n'
'# -*- coding: utf-8 -*-\n'
'# Useless comment\n'
'# Just another useless comment\n'
'# noqa: D400\n'
'"""Module docstring without period"""\n'
)

out, _, code = env.invoke()
assert '' == out
assert code == 0