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

Checks pass when run under 3.8 or later but fails on 3.6 and 3.7 #94

Open
gaborbernat opened this issue Mar 12, 2021 · 4 comments
Open
Labels
bug Something isn't working

Comments

@gaborbernat
Copy link

gaborbernat commented Mar 12, 2021

Bug report

When using this plugin under python3.8 or later it behaves differently than running under python3.6/python3.7.

What's wrong

Take the test project https://github.com/tox-dev/tox/tree/rewrite, using flake8-pytest-style==1.3. When running the flake8 test suite under python3.8 or later it passes with no errors. When running it under 3.7 or 3.6:

src/tox/pytest.py:66:1: PT004 fixture 'ensure_logging_framework_not_altered' does not return anything, add leading underscore
src/tox/pytest.py:111:1: PT004 fixture 'check_os_environ_stable' does not return anything, add leading underscore
src/tox/pytest.py:118:1: PT004 fixture 'no_color' does not return anything, add leading underscore
src/tox/pytest.py:274:1: PT004 fixture 'enable_pep517_backend_coverage' does not return anything, add leading underscore
src/tox/pytest.py:565:1: PT005 fixture '_invalid_index_fake_port' returns a value, remove leading underscore
tests/config/test_sets.py:13:1: PT005 fixture '_conf_builder' returns a value, remove leading underscore

How it should work

Consistently under all python versions.

System information

  • Operating system: Fedora/Ubuntu
  • Python version: Python3.8/3.9/3.7/3.6
  • flake8 version: flake8-bugbear==20.11.1 flake8-comprehensions==3.3.1 flake8-pytest-style==1.3 flake8-spellcheck==0.23 flake8-unused-arguments==0.0.6 flake8==3.8.4

As discovered under tox-dev/tox#1970 by @jugmac00

@gaborbernat gaborbernat added the bug Something isn't working label Mar 12, 2021
@m-burst
Copy link
Owner

m-burst commented Mar 13, 2021

Hi @gaborbernat,
Thank you for the report.
I have reproduced the error locally, will investigate further.

@m-burst
Copy link
Owner

m-burst commented Mar 13, 2021

I have figured out the reason why this inconsistency occurs:

  1. tox has noqa for PT004/PT005 for a few fixtures
  2. We use the lineno of the AST node to check for noqa and to ultimately decide whether to report the error (flake8 does not handle this automatically, so each plugin has to do it by itself)
  3. In 3.8 the reported lineno for function definitions with decorators was changed: before 3.8 it was the line with the first decorator, and since 3.8 it is the line with the def

As a result, under 3.7- we try to find the noqa comment for PT004 on the wrong line, fail to find it and report the error.

I am not sure how to proceed right now, will try to figure out a solution.

In the meantime, the (ugly) workaround is to add a duplicate noqa comment on the fixtures where the PT004/PT005 are reported, e.g.:

@pytest.fixture(autouse=True)  # noqa: PT004 <-- add this for 3.7-
def ensure_logging_framework_not_altered() -> Iterator[None]:  # noqa: PT004  <-- this was already added, works for 3.8+
    ...

@gaborbernat
Copy link
Author

  • We use the lineno of the AST node to check for noqa and to ultimately decide whether to report the error (flake8 does not handle this automatically, so each plugin has to do it by itself)

Do we have an issue with this in flake8? Sounds like something that should be shared across plugins (but ideally provided by flake8).

I am not sure how to proceed right now, will try to figure out a solution.

Sounds to me like you want to make the where to look for noqa dynamic based on the python version.

@m-burst
Copy link
Owner

m-burst commented Mar 14, 2021

Do we have an issue with this in flake8? Sounds like something that should be shared across plugins (but ideally provided by flake8).

I took a closer look, and it seems that flake8 did implement this at some point, so my knowledge on this is not up-to-date.

Regardless of this, flake8 does suffer from the same inconsistency between Python version with some of their checks, see an example issue in their tracker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants