Skip to content

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Sep 15, 2025

Closes #19350

I'm not sure if this is the expected/desired output. Without parentheses around the value, we have:

# unformatted
variable = (
    something # a comment
    .first_method("some string")
)

# formatted
variable = something.first_method("some string")  # a comment

It's a little unclear to me... it almost feels like in both cases (when something does or does not have parentheses) we should maintain the break, since that's what I would expect from a cursory reading of https://docs.astral.sh/ruff/formatter/black/#trailing-end-of-line-comments

@dylwil3 dylwil3 added bug Something isn't working formatter Related to the formatter labels Sep 15, 2025
Copy link
Contributor

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@dylwil3 dylwil3 marked this pull request as ready for review September 15, 2025 15:16
@MichaReiser
Copy link
Member

MichaReiser commented Sep 16, 2025

Thank you.

The comment placement is a bit weird if something isn't parenthesized, but the comment is very long:

variable = (
    (something) # a commentdddddddddddddddddddddddddddddd
    .first_method("some string")
)


variable = (
    something # a commentdddddddddddddddddddddddddddddd
    .first_method("some string")
)

Formatted

variable = (
    (something)  # a commentdddddddddddddddddddddddddddddd
    .first_method("some string")
)


variable = something.first_method(  # a commentdddddddddddddddddddddddddddddd
    "some string"
)

We might want to spent a little time to see if we can preserve the comment placement closer to where it used to be (by e.g. inserting a hard line break after the attribute name or using a line suffix boundary). But this might be harder than it looks and is something we could fix separately.

The fix otherwise looks good, e.g. it also fixes this code that previously produced invalid syntax (may be worth adding as a test to showcase that the issue isn't specific to assignment statements):

if (
    (something)  # a commentdddddddddddddddddddddddddddddd
    .first_method("some string")
): pass

context.source(),
) {
OptionalParentheses::Never
if context.comments().has_trailing(self.value.as_ref()) {
Copy link
Member

@MichaReiser MichaReiser Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll now also use the new layout if value is parenthesized and has a trailing comment like this

variable = (
    (something # a comment  
    ).fist_method("some string")
)

I think that's fine but it might be worth adding a few more tests.

For example, this snippet now gets formatted differently:

if (
    (something # a comment  
    ).fist_method("some string") # second comment
): pass

Overall, maybe add a few more permutations with multiple comments (or at least play with it in the playground)

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 this pull request may close these issues.

Formatter: Parenthesized expression with comment on right hand side in assignment produces invalid syntax

2 participants