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 comment stripped due to syntax error elsewhere in file #4849

Closed
stinodego opened this issue Jun 4, 2023 · 7 comments · Fixed by #4869
Closed

noqa comment stripped due to syntax error elsewhere in file #4849

stinodego opened this issue Jun 4, 2023 · 7 comments · Fixed by #4869
Assignees
Labels
bug Something isn't working suppression Related to supression of violations e.g. noqa

Comments

@stinodego
Copy link
Contributor

stinodego commented Jun 4, 2023

Ruff indicates an unused noqa statement, when in fact it's fine. There is a syntax error in the function definition that triggers this, if the * is removed, everything is fine.

def function(*):
    """Veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery long."""  # noqa: W505

This is on ruff version 0.0.270.

@charliermarsh
Copy link
Member

Thanks, I actually thought we added handling for this case at some point. Will revisit!

@stinodego
Copy link
Contributor Author

Thanks, I actually thought we added handling for this case at some point. Will revisit!

I know, I have reported something similar before:
#2853

Not sure what caused this one to resurface.

@charliermarsh
Copy link
Member

(I believe this regressed when we started generating fixes on every pass, regardless of whether they're applied.)

@charliermarsh
Copy link
Member

@MichaReiser - Any opinion on the right behavior here? The issue is that if we hit a syntax error, we fail to identify the W505 violation in the example above, and so the noqa is considered unused and removed.

We could...

  1. Avoid autofixing at all if we hit a syntax error.
  2. Avoid autofixing RUF100 violations (unused suppression comments) if we hit a syntax error.
  3. Avoid raising RUF100 violations if we hit a syntax error.

@charliermarsh charliermarsh added bug Something isn't working suppression Related to supression of violations e.g. noqa labels Jun 5, 2023
@MichaReiser
Copy link
Member

@charliermarsh My preference is 3. The way I think about this is that rules should only run if their input is correct. What the input is, differs from rule to rule:

  • Physical lines: always valid
  • logical lines: valid tokens
  • AST rules: parsable source tree
  • RUF100: All configured rules ran

That's why I think RUF100 should not run if any of the other phases returned an Err.

@stinodego
Copy link
Contributor Author

For what it's worth, I also believe 3 is the way to go. I don't like all the linting violations popping up in seemingly unrelated places while I'm writing some code.

@charliermarsh charliermarsh self-assigned this Jun 5, 2023
@charliermarsh
Copy link
Member

Sounds good, will fix today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working suppression Related to supression of violations e.g. noqa
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants