Bugfix: Allow comments before module docstring noqa codes #446
Conversation
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): |
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.
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?
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'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?
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.
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
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 added two integration tests:
- Test that a file with only comments does only cause a D100.
- 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.
The behavior is now the same as on master.
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.
see above
@bsamseth Any updates on this? :) |
I just haven't had time to prioritize this myself. You're more than welcome to add the test if you have time, otherwise, I'll get to it at some point. |
Ok i will look into it. But how would I deliver my changes to this PR? Do i need to fork your fork and then you can merge my change into your fork and then it will be delivered here? |
You could if you prefer. I added you as a collaborator to my fork, so you can push to that fork as soon as you accept the invite. Thanks for taking the time! |
see above |
#427 allowed for adding noqa comments to module docstrings. However, this was not respected when there where additional comments before the
# noqa
comment. For example:This still warns about
D400
.This PR fixes this, effectively by discarding leading comments.
Closes #417
Make sure to include the PR number after you open and get one.