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

[BUG] Prospector 1.8 does not respect noqa comments #557

Closed
chatne opened this issue Dec 7, 2022 · 7 comments
Closed

[BUG] Prospector 1.8 does not respect noqa comments #557

chatne opened this issue Dec 7, 2022 · 7 comments
Labels

Comments

@chatne
Copy link

chatne commented Dec 7, 2022

After upgrading from 1.7 to 1.8, prospector does not respect noqa-comments anymore.

How to reproduce

Create an example file to test, example.py:

testVarible = 1

def a():
    pass

Then use Docker to test with both 1.7.7 and 1.8.2 prospector versions:

# Note that pylint version is set here due to https://github.com/PyCQA/prospector/issues/539
docker run --rm --workdir /usr/src/app -v $(pwd):/usr/src/app python:3.11 bash -c \
    "pip install prospector==1.7.7 pylint==2.14.5 && prospector --strictness high example.py"
docker run --rm --workdir /usr/src/app -v $(pwd):/usr/src/app python:3.11 bash -c \
    "pip install prospector==1.8.2 && prospector --strictness high example.py"

There should be 4 errors in both cases:

example.py
  Line: 1
    pycodestyle: N816 / variable 'testVarible' in global scope should not be mixedCase (col 2)
    pylint: invalid-name / Constant name "testVarible" doesn't conform to UPPER_CASE naming style
  Line: 3
    pycodestyle: E302 / expected 2 blank lines, found 1 (col 1)
    pylint: invalid-name / Function name "a" doesn't conform to snake_case naming style

Now add noqa comments

testVarible = 1  # noqa

def a():  # noqa
    pass

And rerun both Docker commands above.

There is 0 errors for 1.7.7, as expected.
With 1.8.2, there are 3 errors for some reason:

example.py
  Line: 1
    pylint: invalid-name / Constant name "testVarible" doesn't conform to UPPER_CASE naming style
  Line: 3
    pycodestyle: E302 / expected 2 blank lines, found 1 (col 1)
    pylint: invalid-name / Function name "a" doesn't conform to snake_case naming style

Whats going on here?

Versions 1.8.0rc0, 1.8.0rc1, 1.8.1 and 1.8.2 all behave the same way. So something changed between 1.7.7 and 1.8.0rc0.

@hugoboos
Copy link

hugoboos commented Dec 7, 2022

Same here. Did some digging.

get_suppressions in suppression.py returns lines_to_ignore. Before 1.8 the key was a str and now it is a Path object.

In postfilter.py on line 46, the path is checked if it is in the dict: if relative_message_path in lines_to_ignore:. There is no match, and thus the error is not suppressed.

Using Python 3.11 on arm64

@hugoboos
Copy link

hugoboos commented Dec 7, 2022

@PetrDlouhy
Copy link

Seems to be still present in 1.8.3. @hugoboos The link may not work as expected - were you able to pinpoint exact commit that caused this? I am not able to determine it from the link.

@Pierre-Sassoulas
Copy link
Collaborator

Pierre-Sassoulas commented Jan 10, 2023

There's no commit to revert as this is due to the str to Path refactor. Something need to be done differently to fix this:

get_suppressions in suppression.py returns lines_to_ignore. Before 1.8 the key was a str and now it is a Path object.

In postfilter.py on line 46, the path is checked if it is in the dict: if relative_message_path in lines_to_ignore:. There is no match, and thus the error is not suppressed.

@stevenxxiu
Copy link

stevenxxiu commented Jan 13, 2023

Here's a bit of a hack fix for people who can't stand this now, and don't want to downgrade Prospector in all their projects.

Update ~/.cache/pre-commit/*/py_env-python3.10/lib/python3.10/site-packages/prospector/postfilter.py:

diff --git a/prospector/postfilter.py b/prospector/postfilter.py
index 17821e3..32840df 100644
--- a/prospector/postfilter.py
+++ b/prospector/postfilter.py
@@ -43,8 +43,8 @@ def filter_messages(filepaths: List[Path], messages: List[Message]) -> List[Mess
             continue
 
         # some lines are skipped entirely by messages
-        if relative_message_path in lines_to_ignore:
-            if message.location.line in lines_to_ignore[relative_message_path]:
+        if Path(message.location.path) in lines_to_ignore:
+            if message.location.line in lines_to_ignore[Path(message.location.path)]:
                 continue
 
         # and some lines have only certain messages explicitly ignored

I say hack here, as the underlying problem isn't fixed. For a proper fix, message.location.path and all the str paths in this project should be converted into Path.

@hugoboos
Copy link

@chatne
Copy link
Author

chatne commented Feb 22, 2023

1.9.0 fixes this issue.

Pull #580 seems to do the trick. Thanks @christokur!

@chatne chatne closed this as completed Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants