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

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

Open
MichaReiser opened this issue Oct 26, 2024 · 3 comments · May be fixed by #14493
Open

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

MichaReiser opened this issue Oct 26, 2024 · 3 comments · May be fixed by #14493
Labels
formatter Related to the formatter help wanted Contributions especially welcome preview Related to preview mode features

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Oct 26, 2024 •
edited by AlexWaygood
Loading

The new preview style formats

"" f'{1:""}'

as

f"{1:\"\"}"

This is correct but results in unnecessary escapes. I think we should account for the quotes in format-specs when choosing the preferred quotes for an f-string.

This can be changed here

ruff/crates/ruff_python_formatter/src/string/normalize.rs

Lines 264 to 295 in 113ce84

if is_f_string_formatting_enabled(context) {
// For f-strings, only consider the quotes inside string-literals but ignore
// quotes inside expressions. This allows both the outer and the nested literals
// to make the optimal local-choice to reduce the total number of quotes necessary.
// This doesn't require any pre 312 special handling because an expression
// can never contain the outer quote character, not even escaped:
// ```python
// f"{'escaping a quote like this \" is a syntax error pre 312'}"
// ```
let mut literals = fstring.elements.literals();
let Some(first) = literals.next() else {
return QuoteMetadata::from_str("", part.flags(), preferred_quote);
};
let mut metadata = QuoteMetadata::from_str(
context.locator().slice(first.range()),
fstring.flags.into(),
preferred_quote,
);
for literal in literals {
metadata = metadata
.merge(&QuoteMetadata::from_str(
context.locator().slice(literal.range()),
fstring.flags.into(),
preferred_quote,
))
.expect("Merge to succeed because all parts have the same flags");
}
metadata

CC: @dhruvmanila
@MichaReiser MichaReiser added formatter Related to the formatter help wanted Contributions especially welcome preview Related to preview mode features labels Oct 26, 2024
@dhruvmanila
Copy link
Member

I don't think we can do that because it would change the value of the format spec itself: https://play.ruff.rs/baf27789-023c-4803-aa8d-e933cc616f0f (refer to the AST).

Note that those quotes are not the surrounding quotes of a literal element but the quotes itself are part of it.

@dhruvmanila
Copy link
Member

Oh wait, I think I misunderstood. Do you mean we should account for the possible quotes inside a format spec, thus choosing single quote for this specific example?

@MichaReiser
Copy link
Member Author

Oh wait, I think I misunderstood. Do you mean we should account for the possible quotes inside a format spec, thus choosing single quote for this specific example?

Yes. We should use single quotes for the outer f-string because it avoids escaping the format-spec (unless the f-string contains more single quotes outside the format spec)

@MichaReiser MichaReiser linked a pull request Nov 20, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
formatter Related to the formatter help wanted Contributions especially welcome preview Related to preview mode features
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consider quotes inside format-specs when choosing the quotes for an f-string astral-sh/ruff
2 participants
@MichaReiser @dhruvmanila