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
Merged

Bugfix: Allow comments before module docstring noqa codes #446

merged 10 commits into from Jul 15, 2020

Conversation

bsamseth
Copy link
Contributor

@bsamseth bsamseth commented Jan 9, 2020

#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:

# -*- coding: utf-8 -*-
# noqa: D400
"""No period at the end""" 

This still warns about D400.

This PR fixes this, effectively by discarding leading comments.
Closes #417

  • Add unit tests and integration tests where applicable.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

@bsamseth bsamseth changed the title Allow comments before module docstring noqa codes Bugfix: Allow comments before module docstring noqa codes Jan 9, 2020
@bsamseth bsamseth mentioned this pull request Jan 9, 2020
2 tasks
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.

src/pydocstyle/parser.py Show resolved Hide resolved
@bsamseth bsamseth requested a review from samj1912 March 6, 2020 13:26
Copy link
Member

@samj1912 samj1912 left a comment

Choose a reason for hiding this comment

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

see above

@Cielquan
Copy link
Contributor

Cielquan commented Jun 3, 2020

@bsamseth Any updates on this? :)

@bsamseth
Copy link
Contributor Author

bsamseth commented Jun 3, 2020

@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.

@Cielquan
Copy link
Contributor

Cielquan commented Jun 3, 2020

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

@bsamseth
Copy link
Contributor Author

bsamseth commented Jun 3, 2020

@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!

@Cielquan
Copy link
Contributor

Cielquan commented Jun 4, 2020

see above

@bsamseth bsamseth requested a review from samj1912 June 4, 2020 09:49
@samj1912 samj1912 merged commit 2dfbb38 into PyCQA:master Jul 15, 2020
@bsamseth
Copy link
Contributor Author

bsamseth commented Jul 15, 2020

🎉 It only took us 6 months, but it is finally in! Looking forward to integrating pydocstyle in our codebase, finally. Thanks for your time reviewing @samj1912, and thanks @Cielquan for pitching in with the tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noqa cannot be used in module docstrings
3 participants