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

`noqa` comments are detect as code #103

Closed
tbrlpld opened this issue Dec 7, 2019 · 11 comments
Closed

`noqa` comments are detect as code #103

tbrlpld opened this issue Dec 7, 2019 · 11 comments
Labels
bug

Comments

@tbrlpld
Copy link
Contributor

@tbrlpld tbrlpld commented Dec 7, 2019

This line raises an E800 warning.

# noqa: A100

These do not:

# noqa: 100
# noqa: something
# noqa: "Something"

If anything, the second block might be code (like a line of a multi-line dictionary definition). The first block is a valid noqa-comment and should not raise that warning.

@sobolevn

This comment has been minimized.

Copy link
Owner

@sobolevn sobolevn commented Dec 8, 2019

@tbrlpld sorry, but this is not something we can work on our side. That's eradicate's issue.
Can you please reopen it in the upstream repo?

@sobolevn sobolevn closed this Dec 8, 2019
@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 9, 2019

@sobolevn Just realized I was using it wrong anyways. The # noqa: should be an inline comment anyways. When it is used that way, it is not recognized as code.

I cam across this because of the DAR docstring violations when I want to ignore these violations on a multi-line docstring.
This seems to a flake8 related though. I am not sure how can you define to ignore a rule on a multi-line docstring.

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 9, 2019

Ok, so darglint ignores errors when the # noqa: is just embedded in the docstring like so (
https://github.com/terrencepreilly/darglint#ignoring-errors-in-a-docstring):

def myfunc():
    """
    Return true, always.

    Here is some more stuff.

    # noqa: DAR201
    """
    return True

This suppresses the specified error DAR201 as desired.

But, E800 is raised by this Flake8 plugin.

eradicate itself does not do anything here (not even with the aggressive -a setting enabled). I assume it just ignores everything in a docstring.

The same is true for multi-line strings. E800 is raised, but eradicate does not do anything here.

mystring = """Return true, always.

Here is some more stuff.

# noqa: DAR201
"""

@sobolevn do you still consider this an issue for eradicate or of flake8-eradicate?

@sobolevn sobolevn reopened this Dec 10, 2019
@sobolevn

This comment has been minimized.

Copy link
Owner

@sobolevn sobolevn commented Dec 10, 2019

Ok, this is our issue indeed. Why? Because we use physical_line https://github.com/sobolevn/flake8-eradicate/blob/master/flake8_eradicate.py#L35 instead of logical_line

Sorry! 🙂

Will you please fix it?

@sobolevn sobolevn added the bug label Dec 10, 2019
@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 11, 2019

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 12, 2019

After finally getting pyenv working with poetry I have an issue trying to run mypy on the master with not changes.

The contribution guidelines say to run mypy wemake_python_styleguide. This does not work, because there is no such file or directory.

$ mypy wemake_python_styleguide
mypy: can't read file 'wemake_python_styleguide': No such file or directory

I assume it should be

$ mypy flake8_eradicate.py
Success: no issues found in 1 source file

Is that correct? Should I open another issue to tackle that?

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 13, 2019

@sobolevn logical_line does not work. It breaks the whole program, because logical lines do not consider commented lines.

tbrlpld added a commit to tbrlpld/flake8-eradicate that referenced this issue Dec 13, 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`.
sobolevn#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.
@sobolevn

This comment has been minimized.

Copy link
Owner

@sobolevn sobolevn commented Dec 13, 2019

Hm, any ideas how this can be solved?

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 13, 2019

@sobolevn

This comment has been minimized.

Copy link
Owner

@sobolevn sobolevn commented Dec 13, 2019

Source code is always helpful for me: https://github.com/PyCQA/flake8/blob/6cc0abbea2e2f44977c7ac5e75072e0ca20c0831/src/flake8/checker.py#L514
It is quite easy to follow and can be modified locally to test things.

@tbrlpld

This comment has been minimized.

Copy link
Contributor Author

@tbrlpld tbrlpld commented Dec 17, 2019

@sobolevn Thanks for pointing that out. That definitely helped.

I found a solution by utilizing the tokens that can be requested as input from flake8.
See my pull request #106

Hope this is fine by you.

sobolevn added a commit that referenced this issue Dec 17, 2019
* Add false positive test for noqa comment

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

* Check if comment in physical line before eradicate

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.

* Rename function and add docstring

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

* Remove single type annotation

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 closed this Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.