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

Fix unstable f-string formatting for expressions containing a trailing comma #15545

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

MichaReiser
Copy link
Member

Summary

Fixes #15536

The RemoveSoftlinesBuffer removes soft line breaks and if_group_breaks elements but it
doesn't remove expand_parent elements. This can lead to instable formatting
when f-strings are wrapped in a group because it makes that group expand. For example,
print(f"{ {}, 1, }") first gets formatted to

print(
    f"{ {}, 1 }"
)

and only then converges at print(f"{ {}, 1 }").

I first tried to also remove expand_parent elemnents in the RemoveSoftLinesBuffer
but that results in many failing tests around assignment formatting because
we rely on Document::will_break to determine if we should try the flat
layout and that only returns true if the f-string formatting contains the expand_parent along the trailing comma.

I reviewed all expand_parent usages and realized that the trailing comma is the only
usage in our expression formatting. That's why I decided to adjust the trailing comma
formatting instead to omit the trailing comma if we know that we're in a flat f-string.

This is also how Dhruv implemented it originally before I removed the check in #14489 because I thought it
it no longer necessary.

Test Plan

Added regression test

@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Jan 17, 2025
@MichaReiser MichaReiser marked this pull request as ready for review January 17, 2025 07:53
@MichaReiser MichaReiser force-pushed the micha/f-string-trailing-comma branch from bcc9793 to 9b94a2e Compare January 17, 2025 07:57
@MichaReiser MichaReiser force-pushed the micha/f-string-trailing-comma branch from 9b94a2e to 3347f57 Compare January 17, 2025 08:00
Base automatically changed from micha/f-string-tuple to main January 17, 2025 08:02
Copy link
Contributor

github-actions bot commented Jan 17, 2025

ruff-ecosystem results

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

@MichaReiser MichaReiser force-pushed the micha/f-string-trailing-comma branch from 3347f57 to 7146658 Compare January 17, 2025 08:14
@dhruvmanila dhruvmanila changed the title Fix instable f-string formatting for expressions containing a trailing comma Fix unstable f-string formatting for expressions containing a trailing comma Jan 17, 2025
@MichaReiser MichaReiser merged commit 1ecb7ce into main Jan 17, 2025
21 checks passed
@MichaReiser MichaReiser deleted the micha/f-string-trailing-comma branch January 17, 2025 09:08
dcreager added a commit that referenced this pull request Jan 17, 2025
* main:
  [red-knot] Inline `SubclassOfType::as_instance_type_of_metaclass()` (#15556)
  [`flake8-comprehensions`] strip parentheses around generators in `unnecessary-generator-set` (`C401`) (#15553)
  [`pylint`] Implement `redefined-slots-in-subclass` (`W0244`) (#9640)
  [`flake8-bugbear`] Do not raise error if keyword argument is present and target-python version is less or equals than 3.9 (`B903`) (#15549)
  [red-knot] `type[T]` is disjoint from `type[S]` if the metaclass of `T` is disjoint from the metaclass of `S` (#15547)
  [red-knot] Pure instance variables declared in class body (#15515)
  Update snapshots of #15507 with new annotated snipetts rendering (#15546)
  [`pylint`] Do not report methods with only one `EM101`-compatible `raise` (`PLR6301`) (#15507)
  Fix unstable f-string formatting for expressions containing a trailing comma (#15545)
  Support `knot.toml` files in project discovery (#15505)
  Add support for configuring knot in `pyproject.toml` files (#15493)
  Fix bracket spacing for single-element tuples in f-string expressions (#15537)
  [`flake8-simplify`] Do not emit diagnostics for expressions inside string type annotations (`SIM222`, `SIM223`) (#15405)
  [`flake8-pytest-style`] Do not emit diagnostics for empty `for` loops (`PT012`, `PT031`) (#15542)
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.

F-string formatting: Instable formatting for tuple with trailing comma
2 participants