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

ruff format moves # noqa comment to wrong location #10281

Closed
jakob-keller opened this issue Mar 7, 2024 · 4 comments · Fixed by #10297
Closed

ruff format moves # noqa comment to wrong location #10281

jakob-keller opened this issue Mar 7, 2024 · 4 comments · Fixed by #10297
Labels
bug Something isn't working formatter Related to the formatter

Comments

@jakob-keller
Copy link

I would like to switch my codebase to ruff format, but there is one annoying issue that breaks my use case: Some # noqa comments are moved resulting in issues when calling ruff check.

Hers's a minimal code snippet to demonstrate what happens:

# test.py (original)

import typing


def foo(
    __bar: str,
    /,
    **specifiers: typing.Any,  # noqa: ANN401
) -> int:
    return len(specifiers)
$ ruff --version
ruff 0.3.1

$ ruff check --isolated --select ANN401 test.py

$ ruff format --isolated test.py
1 file reformatted
# test.py (reformatted)

import typing


def foo(
    __bar: str,
    /,  # noqa: ANN401
    **specifiers: typing.Any,
) -> int:
    return len(specifiers)
$ ruff check --isolated --select ANN401 test.py
test.py:7:19: ANN401 Dynamically typed expressions (typing.Any) are disallowed in `**specifiers`
Found 1 error.

I assume this issue is an edge case that has to do with the keyword-only / syntax being used. Are there any workarounds available?

@charliermarsh charliermarsh added suppression Related to supression of violations e.g. noqa formatter Related to the formatter bug Something isn't working labels Mar 7, 2024
@charliermarsh
Copy link
Member

Oh hmm, this looks like a bug, thanks. Will look into it. The only workaround I could think of is using # fmt: skip on the header for now:

import typing


def foo(
    __bar: str,
    /,
    **specifiers: typing.Any,  # noqa: ANN401
) -> int:  # fmt: skip
    return len(specifiers)

@charliermarsh
Copy link
Member

This is why we need a programming language that doesn't allow comments 😂

@MichaReiser MichaReiser removed the suppression Related to supression of violations e.g. noqa label Mar 8, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Mar 8, 2024

This is why we need a programming language that doesn't allow comments 😂

Or an AST that represents / ;)

FYI: This is not specific to noqa. We incorrectly associate any end-of-line comment **kwargs comment following /

https://play.ruff.rs/cb2a370c-ae53-48a5-a35f-810b9c537ae2

I suspect that the error is somewhere in

fn handle_parameters_separator_comment<'a>(

MichaReiser added a commit that referenced this issue Mar 8, 2024
## Summary

Fixes the handling end of line comments that belong to `**kwargs` when
the `**kwargs` come after a slash.

The issue was that we missed to include the `**kwargs` start position
when determining the start of the next node coming after the `/`.

Fixes #10281

## Test Plan

Added test
@jakob-keller
Copy link
Author

Thanks @MichaReiser and everyone else at https://github.com/astral-sh! You rock 🚀

nkxxll pushed a commit to nkxxll/ruff that referenced this issue Mar 10, 2024
## Summary

Fixes the handling end of line comments that belong to `**kwargs` when
the `**kwargs` come after a slash.

The issue was that we missed to include the `**kwargs` start position
when determining the start of the next node coming after the `/`.

Fixes astral-sh#10281

## Test Plan

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

Successfully merging a pull request may close this issue.

3 participants