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

Formatter changes order of comments. #11569

Closed
randolf-scholz opened this issue May 27, 2024 · 6 comments · Fixed by #11632
Closed

Formatter changes order of comments. #11569

randolf-scholz opened this issue May 27, 2024 · 6 comments · Fixed by #11632
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@randolf-scholz
Copy link

randolf-scholz commented May 27, 2024

class Foo:
    # comment 1
    @abstractmethod
    def foo(self) -> None: ...
    @abstractmethod
    def bar(self) -> None: ...
    # comment 2

    # comment 3
    def baz(self) -> None:
        return None
    # comment 4

becomes

class Foo:
    # comment 1
    @abstractmethod
    def foo(self) -> None: ...
    @abstractmethod
    def bar(self) -> None: ...

    # comment 3
    # comment 2
    def baz(self) -> None:
        return None

    # comment 4

[ruff-playground]

  1. Comments 2 and 3 get swapped.
  2. An extra space is inserted after def bar. Since these are stubs, no space should be inserted. (for instance, comment 1/2 can be # region/#endregion). But this is just my opinion.
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels May 27, 2024
@MichaReiser MichaReiser self-assigned this May 27, 2024
@randolf-scholz
Copy link
Author

randolf-scholz commented May 27, 2024

Another example, where the comment actually moves inside another function def.

class Foo:
    @abstractmethod
    def foo(self) -> None: ...
    # comment 1

    def baz(self) -> None:
        return None

becomes

class Foo:
    @abstractmethod
    def foo(self) -> None: ...

    def baz(self) -> None:
    # comment 1
        return None

https://play.ruff.rs/973d0af8-e586-4fc0-9aec-28fecf0dc047

@MichaReiser
Copy link
Member

MichaReiser commented May 31, 2024

You're good at finding comment reordering bugs and thank you so much at taking the time to report them!

Uhh, the second one is really bad. Let me have a look.

What's interesting is that all comments are associated with the right method. So this is a rendering bug and not a placement bug (logic where we figure out where a comment is most likely to belong).

https://play.ruff.rs/5e5ae359-b017-4d82-ac47-2c77f254f161

@MichaReiser
Copy link
Member

To make it worse, this is not even related to classes nor does it require decorators. https://play.ruff.rs/2bcde006-72e6-409f-9fa9-659ef78d7944

@randolf-scholz
Copy link
Author

I would suspect this is somehow related to special logic for formatting stub implementations #8357

@MichaReiser
Copy link
Member

Yeah, that's a great observation. The issue is that, for stubs, we don't write extra empty lines between the comment and the next function, which drips over the formatting.

@MichaReiser
Copy link
Member

@randolf-scholz thanks for debugging this together. The fix turned out to be not that bad :)

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.

2 participants