Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolves Issue #103 noqa comments #106

Merged
merged 4 commits into from Dec 17, 2019

Conversation

@tbrlpld
Copy link
Contributor

tbrlpld commented Dec 17, 2019

Previously, noqa comments inside of a multi-line docstring where flagged as commented out code. When using eradicate directly on a file which includes such a noqa comment it does not show a violation.

Example of such a multi-line docstring:

def some_function():
    """
    Test for noqa comments in docstrings.

    This function has a multi-line doc string, but no return value is
    stipulated, while the function defines a return. This would raise DAR201
    flake8 violation. To suppress this raise violation the following noqa
    comment is defined in the docstring.

    # noqa: DAR201

    This noqa comment should not be detected as commented out code. `eradicate`
    itself does not raise this as a violation.

    """
    return "something"

This kind of noqa comments are needed to suppress violations raised by darglint, e.g. violations associated with missing return statements in the docstring.

This issue is caused by passing isolated physical lines of code to the eradicate function filter_commented_out_code. That function does expect to operate on a file's whole source code rather than on isolated lines of code. From the whole source code the context of each line can be determined. This allows differentiation between actual comment lines and lines that only look like comments.

To resolve this issue, while still keeping the checker on one physical line at a time, a preliminary test is introduced that checks if the current physical line contains a comment according to the tokens of the physical line. These tokens are available as input to the plugin from flake8.

Solves #103

tbrlpld added 3 commits Dec 12, 2019
`flake8-eradicate` raises a violation when a `# noqa` comment is defined
inside of a multi-line docstring. `eradicate` itself does not raise this
as a violation.

It should not be detected as a violation, because it is also normal way
to suppress `flake8` violations (like DAR201) in multi-line docstrings.

As @sobolevn points out, this is probably related to the use of
`physical_line` rather than `logical_line`.
#103 (comment)

This commit adds a function definition to the `correct.py` fixture
that has a multi-line docstring and a `# noqa` comment defined inside of
it.
When passing a physical line to eradicate, we need to make sure this
line contains a comment (as per Python definition). Otherwise this
could lead to documentation or noqa-comments in multi-line docstrings as
being interpreted as a comment. This is probably caused by passing a
single physical line to the eradicate function
`filter_commented_out_code`, while this function expects a whole file as
input. When passing a whole files source into the
`filter_commented_out_code` can parse the logical structure it self.

Since we are removing the context for the `filter_commented_out_code`
function, we need to make sure the passed in line actually contains a
comment. This is a test, that would usually happen inside of `eradicate`
but requires more context.

To test if the physical line contains a comment, we can request the
`tokens` of the current physical line from `flake8` to be passed in.
The tokens allow to pre-check if the physical line actually contains
a comment. The eradicate function is only invoked for physical lines
that do contain a comment.
The function `_is_equal_source` is renamed to
`_contains_commented_out_code`. This change reflects the larger scope
that the function now has.

The function was previously only checking the difference between the
filtered source and the physical line.

Now the function also needs to test if the physical line even contains
a comment before the line is reduced by the eradicate function.
This scope is a bit larger which is reflected in the new name.

The previous name `_is_equal_source` was derived from the inner
implementation. To me it was not immediately clear why we are testing
for "source equality". The new name `_contains_commented_out_code`
describes what the function can tell us, but now how it is implemented.
The implementation is outside of the concern of the caller. All that
the caller is interested in is, if the current physical line contains
a comment or not.

**Attention** With the change of the name, the logic is also inverted.
While `_is_equal_source` was `True` when the original and the filtered
line are the same, `_contains_commented_out_code` is `True` when they
are different (because the commented code was removed by eradicate).
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 17, 2019

Pull Request Test Coverage Report for Build 340

  • 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 98.113%

Totals Coverage Status
Change from base Build 338: 0.7%
Covered Lines: 42
Relevant Lines: 43

💛 - Coveralls
Copy link
Owner

sobolevn left a comment

Awesome fix! Thanks a lot!

flake8_eradicate.py Outdated Show resolved Hide resolved
Because no other type annotations are used in the method, the one
used type annotation breaks the consistency of the method.

The type annotation is removed.
@sobolevn sobolevn merged commit aa76a84 into sobolevn:master Dec 17, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage increased (+0.7%) to 98.113%
Details
@sobolevn

This comment has been minimized.

Copy link
Owner

sobolevn commented Dec 17, 2019

Thanks a lot, @tbrlpld

P.S. Awesome commit messages! 👍

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

tbrlpld commented Dec 17, 2019

@sobolevn Thank you 😄

Always happy to help when I can.

@tbrlpld tbrlpld deleted the tbrlpld:issue-103-noqa-comments branch Dec 17, 2019
@sobolevn

This comment has been minimized.

Copy link
Owner

sobolevn commented Dec 17, 2019

I will release a new version tomorrow.

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

tbrlpld commented Dec 17, 2019

Awesome! I am looking forward to using it 😄 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.