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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 4 commits into from
+329 −80

Conversation

MichaReiser
Copy link
Member

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 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

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

@MichaReiser
Copy link
Member Author

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

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

dhruvmanila
dhruvmanila reviewed Nov 21, 2024
View reviewed changes
crates/ruff_python_formatter/src/string/normalize.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/string/normalize.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/string/normalize.rs Show resolved Hide resolved
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

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

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.

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