Skip to content

Comments

Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values#16187

Merged
MichaReiser merged 2 commits intomainfrom
micha/fix-instable-trailing-attribute-value-formatting
Feb 18, 2025
Merged

Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values#16187
MichaReiser merged 2 commits intomainfrom
micha/fix-instable-trailing-attribute-value-formatting

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 16, 2025 •
edited
Loading

Summary

Fixes #16151

This fixes a formatter instability where it incorrectly moved a trailing end-of-line comment
of a parenthesized attribute value was moved to the next line:

result = (
    (await query_the_thing(mypy_doesnt_understand))  # type: ignore[x]
    .foo()
    .bar()
)

Got reformatted to

result = (
    (await query_the_thing(mypy_doesnt_understand))
      # type: ignore[x]
    .foo()
    .bar()
)

and then finally to:

result = (
    (await query_the_thing(mypy_doesnt_understand))
    # type: ignore[x]
    .foo()
    .bar()
)

Not only is it an instability, but it also moves the type: ignore comment away from the node it was attributing.

This PR fixes both by preserving the input formatting. Both those examples are correct formattings now:

result = (
    (await query_the_thing(mypy_doesnt_understand))  # type: ignore[x]
    .foo()
    .bar()
)


```py
result = (
    (await query_the_thing(mypy_doesnt_understand))  # type: ignore[x]
    .foo()
    .bar()
)

result = (
    (
        await query_the_thing(mypy_doesnt_understand)  # type: ignore[x]
    )  # other
    .foo()
    .bar()
)

I don't expect this to change any stable formatting because the formatter always moved trailing end-of-line comments and made them leading own-line comments of the . token

Test Plan

Added tests, ecosystem check

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2025 •
edited
Loading

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Feb 16, 2025
@AlexWaygood AlexWaygood changed the title Fix instable formatting of trailing end-of-line comments of parenthesized attribute values Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values Feb 16, 2025
@huonw
Copy link

huonw commented Feb 16, 2025

Thanks for fixing this! (BTW, I think the code examples in the PR description aren't quite the right ones.) 😄

@MichaReiser MichaReiser force-pushed the micha/fix-instable-trailing-attribute-value-formatting branch from 28f8fc4 to 66a25a5 Compare February 17, 2025 07:27
@MichaReiser MichaReiser force-pushed the micha/fix-instable-trailing-attribute-value-formatting branch from 66a25a5 to bab82b7 Compare February 17, 2025 13:58
@MichaReiser
Copy link
Member Author

MichaReiser commented Feb 17, 2025

@huonw thanks for making me aware. I was in the middle of writing the summary and then realized that the fix wasn't ideal. So I decided to push what I had.

MichaReiser
MichaReiser commented Feb 17, 2025
View reviewed changes
crates/ruff_python_formatter/src/comments/placement.rs
{
return if comment.start() < right_paren.start() {
CommentPlacement::trailing(attribute.value.as_ref(), comment)
} else {
Copy link
Member Author

@MichaReiser MichaReiser Feb 17, 2025

Choose a reason for hiding this comment

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

We want to fall through to the next case if the comment is after the closing parentheses

@MichaReiser MichaReiser marked this pull request as ready for review February 17, 2025 14:05
dhruvmanila
dhruvmanila approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

I'm a bit unfamiliar with fluent chain layout so it might good to get a second pair of eyes on this (Konsti?) but otherwise looks good.

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py
Comment on lines +164 to +171
(
(
a # trailing end-of-line
# trailing own-line
) # trailing closing parentheses
# dangling before dot
.b # trailing end-of-line
)
Copy link
Member

@dhruvmanila dhruvmanila Feb 18, 2025

Choose a reason for hiding this comment

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

What's the significance of this test case? It seems to be working fine on main

Copy link
Member Author

@MichaReiser MichaReiser Feb 18, 2025

Choose a reason for hiding this comment

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

I mainly wanted to make sure that I don't break this case.

@MichaReiser
Copy link
Member Author

MichaReiser commented Feb 18, 2025

I'm a bit unfamiliar with fluent chain layout so it might good to get a second pair of eyes on this (Konsti?) but otherwise looks good.

Yeah. I don't think anyone is very familiar with this specific code at the moment :)

@MichaReiser MichaReiser merged commit 31180a8 into main Feb 18, 2025
21 checks passed
@MichaReiser MichaReiser deleted the micha/fix-instable-trailing-attribute-value-formatting branch February 18, 2025 07:43
dcreager added a commit that referenced this pull request Feb 18, 2025
@dcreager
Merge branch 'main' into alex/eager-scopes
5db470e 
* main: (60 commits)
  [`refurb`] Manual timezone monkeypatching (`FURB162`) (#16113)
  [`pyupgrade`] Do not upgrade functional TypedDicts with private field names to the class-based syntax (`UP013`) (#16219)
  Improve docs for PYI019 (#16229)
  Refactor `CallOutcome` to `Result` (#16161)
  Fix minor punctuation errors (#16228)
  Include document specific debug info (#16215)
  Update server to return the debug info as string (#16214)
  [`airflow`] Group `ImportPathMoved` and `ProviderName` to avoid misusing (`AIR303`) (#16157)
  Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values (#16187)
  Ignore source code actions for a notebook cell (#16154)
  Add FAQ entry for `source.*` code actions in Notebook (#16212)
  red-knot: move symbol lookups in `symbol.rs` (#16152)
  better error messages while loading configuration `extend`s (#15658)
  Format `index.css` (#16207)
  Improve API exposed on `ExprStringLiteral` nodes (#16192)
  Update Rust crate tempfile to v3.17.0 (#16202)
  Update cloudflare/wrangler-action action to v3.14.0 (#16203)
  Update NPM Development dependencies (#16199)
  Update Rust crate smallvec to v1.14.0 (#16201)
  Update Rust crate codspeed-criterion-compat to v2.8.0 (#16200)
  ...
@BrewTestBot BrewTestBot mentioned this pull request Feb 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhruvmanila dhruvmanila dhruvmanila approved these changes

Assignees

No one assigned

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Non-idempotent comment movement for ((x) # comment here ...) with parens and 3+ lines

3 participants

@MichaReiser @huonw @dhruvmanila