Skip to content

Consider quotes inside format-specs when choosing the quotes for an f-string#14493

Merged
MichaReiser merged 4 commits intomainfrom
micha/f-string-expressions-format-spec
Nov 22, 2024
Merged

Consider quotes inside format-specs when choosing the quotes for an f-string#14493
MichaReiser merged 4 commits intomainfrom
micha/f-string-expressions-format-spec

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Nov 20, 2024

Summary

Fixes #13935

Test Plan

Added test

@MichaReiser MichaReiser added formatter Related to the formatter preview Related to preview mode features labels Nov 20, 2024
@MichaReiser MichaReiser force-pushed the micha/f-string-expressions-format-spec branch from 68ca92e to f681076 Compare November 20, 2024 15:50
@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2024 •
edited
Loading

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 20, 2024

Hmm, this now leads to invalid syntax for "aaa " f'{1=: "abcd {'aa'}}'

@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 20, 2024

Okay, this is a pre-existing issue, but not one that gets fixed by this PR.

f'{1=: "abcd {'aa'} \'\'}'

We can't change the outer quotes because the format-spec quotes then become the "closing" quotes of the entire-fstring (which they shouldn't). That means we have to preserve the quotes if:

  • an expression has a format spec that contains the current quote character
  • the expression uses debug text

@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 20, 2024

I'll do another review myself of the code changes with a fresh mind tomorrow morning but I think this is mostly correct.

MichaReiser
MichaReiser commented Nov 20, 2024
View reviewed changes
dhruvmanila
dhruvmanila reviewed Nov 21, 2024
View reviewed changes
crates/ruff_python_formatter/src/string/implicit.rs
Comment on lines +249 to +250
let quote = if let Some(quote) = preserve_quotes_requirement {
quote
Copy link
Member

@dhruvmanila dhruvmanila Nov 21, 2024

Choose a reason for hiding this comment

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

The only change here is the addition of this condition to return the quote that needs to be preserved? I'm curious to know why was this condition not present before this? It seems like it should've been present.

Copy link
Member Author

@MichaReiser MichaReiser Nov 21, 2024

Choose a reason for hiding this comment

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

Not 100%. I would have to remind myself of how the pre 312 fstring formatting works again. It might also just be that we were missing a test.

@MichaReiser MichaReiser force-pushed the micha/f-string-expressions-format-spec branch from c402856 to 2e3416e Compare November 22, 2024 12:39
@MichaReiser MichaReiser enabled auto-merge (squash) November 22, 2024 12:39
@MichaReiser MichaReiser merged commit b80de52 into main Nov 22, 2024
@MichaReiser MichaReiser deleted the micha/f-string-expressions-format-spec branch November 22, 2024 12:43
@MichaReiser MichaReiser mentioned this pull request Nov 26, 2024
Closed
@BrewTestBot BrewTestBot mentioned this pull request Nov 29, 2024
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

formatter Related to the formatter preview Related to preview mode features

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

f-string formatting: Consider format specs when choosing the preferred quotes

2 participants

@MichaReiser @dhruvmanila