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
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2025

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)

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to limit this to end of line comments only? Can you add a test where a value has both (unless that's not possible)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible to have both (and we get a syntax error on main after formatting, but not in this PR). But you're right that we only need to use Multiline here if there's a trailing end of line comment, so I've narrowed the condition.

Added a test with both and a test just with trailing own-line comment (the latter formats with the same IR as on main, and without a syntax error, now that we narrowed the condition).

@dylwil3 dylwil3 force-pushed the parens-comment-attr-fmt-bug branch from 080b026 to 4fe83ce Compare November 14, 2025 22:23
@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 14, 2025

I've added some more tests, and I'm personally okay with the edge cases that produce slightly awkward formatting (in the name of getting rid of this syntax error). But let me know if you want me to try to tweak anything as part of this PR.

@dylwil3 dylwil3 requested a review from MichaReiser November 14, 2025 22:24
context.source(),
) {
OptionalParentheses::Never
if context.comments().has_trailing(self.value.as_ref()) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to limit this to end of line comments only? Can you add a test where a value has both (unless that's not possible)

@dylwil3 dylwil3 merged commit 8156b45 into astral-sh:main Nov 17, 2025
37 checks passed
dcreager added a commit that referenced this pull request Nov 17, 2025
* origin/main: (26 commits)
  Mention `force-exclude` in "Configuration > Python file discovery" (#21500)
  Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418)
  [ty] suppress invalid suggestions in import statements (#21484)
  Limit `eglot-format` hook to eglot-managed Python buffers (#21459)
  Adjust own-line comment placement between branches (#21185)
  [ty] Subscript assignment diagnostics follow-up (#21452)
  [ty] Inlay hint call argument location (#20349)
  [ty] Use `CompactStr` for `StringLiteralType` (#21497)
  Update CodSpeedHQ/action action to v4.3.4 (#21488)
  Update salsa digest to a885bb4 (#21486)
  Update dependency ruff to v0.14.5 (#21489)
  Update astral-sh/setup-uv action to v7.1.3 (#21487)
  Update Rust crate get-size2 to v0.7.2 (#21490)
  Update Rust crate indicatif to v0.18.3 (#21491)
  Update Rust crate quick-junit to v0.5.2 (#21492)
  Update taiki-e/install-action action to v2.62.52 (#21493)
  Fix analyze graph tests on windows (#21481)
  `analyze`: Add option to skip over imports in `TYPE_CHECKING` blocks (#21472)
  [ty] Dataclasses: `__hash__` semantics and `unsafe_hash` (#21470)
  [ty] Dataclass transform: complete set of parameters (#21474)
  ...
dcreager added a commit that referenced this pull request Nov 17, 2025
* dcreager/coolable: (31 commits)
  mdformat
  don't panic
  Mention `force-exclude` in "Configuration > Python file discovery" (#21500)
  Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418)
  [ty] suppress invalid suggestions in import statements (#21484)
  known discrepancy TODO
  α-rename todo
  equiv too
  Limit `eglot-format` hook to eglot-managed Python buffers (#21459)
  Adjust own-line comment placement between branches (#21185)
  [ty] Subscript assignment diagnostics follow-up (#21452)
  [ty] Inlay hint call argument location (#20349)
  [ty] Use `CompactStr` for `StringLiteralType` (#21497)
  Update CodSpeedHQ/action action to v4.3.4 (#21488)
  Update salsa digest to a885bb4 (#21486)
  Update dependency ruff to v0.14.5 (#21489)
  Update astral-sh/setup-uv action to v7.1.3 (#21487)
  Update Rust crate get-size2 to v0.7.2 (#21490)
  Update Rust crate indicatif to v0.18.3 (#21491)
  Update Rust crate quick-junit to v0.5.2 (#21492)
  ...
@ntBre ntBre mentioned this pull request Nov 18, 2025
31 tasks
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