Add f-string consistent quotes formatting option#23098
Add f-string consistent quotes formatting option#23098matthewlloyd wants to merge 2 commits intoastral-sh:mainfrom
Conversation
Introduces a new formatter option `f-string-consistent-quotes` that leverages Python 3.12's PEP 701 to use consistent quotes inside f-string expressions rather than alternating quote styles for compatibility. When enabled and targeting Python 3.12+, the formatter will use the same quote style (following the `quote-style` setting) inside f-string expressions as in the outer f-string. This produces more consistent and readable code. When disabled (default) or targeting Python versions below 3.12, the formatter will continue to alternate quotes for compatibility.
|
ntBre
left a comment
There was a problem hiding this comment.
Thank you for reviving this work! Just wanted to note that Micha is out this month, and I'd love to get his input on this, so it may be a few weeks before a review.
I skimmed through quickly, and the changes look reasonable to me at a high level besides the small comments I left.
There was a problem hiding this comment.
Thank yous This looks good to me. I only have a small nit code wise.
But I want to do some bikeshedding on the option name. Similar to quote-style, I think I'd prefer an option with two string values. I also think it's important that the option isn't f-string specific because it also applies to t-strings. This will make naming even trickier
nested-interpolated-strings-quote-style = "preferred" | "alternate"interpolated-strings-consistent-quote-style = true | falsenested-template-string-quote-style = "preferred" | "alternate"interpolated-string-quote-style = "consistent" | "alternate"
But I must admit, I don't really like any of them. They all sound very obscure. So I'd be interested to get some more opinions.
Are there cases where the "consistent" styling would be different from the preferred quote style?
| // When f_string_consistent_quotes is enabled AND we're targeting Python 3.12+, | ||
| // use the preferred quote style consistently. | ||
| if supports_pep_701 && consistent_quotes && !preferred_quote_style.is_preserve() { | ||
| return preferred_quote_style; | ||
| } | ||
| // Otherwise, use alternating quotes for compatibility. | ||
| // This logic is even necessary when using preserve and the target python version doesn't support PEP701 because | ||
| // we might end up joining two f-strings that have different quote styles, in which case we need to alternate the quotes | ||
| // for inner strings to avoid a syntax error: `string = "this is my string with " f'"{params.get("mine")}"'` |
There was a problem hiding this comment.
Nit: I'd probably restructure the code here as in this patch to remove some of the repeated conditions:
Index: crates/ruff_python_formatter/src/string/normalize.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/ruff_python_formatter/src/string/normalize.rs b/crates/ruff_python_formatter/src/string/normalize.rs
--- a/crates/ruff_python_formatter/src/string/normalize.rs (revision 5329113011ab9fdb93302c660f5a03052f03bbfe)
+++ b/crates/ruff_python_formatter/src/string/normalize.rs (date 1772639527311)
@@ -10,10 +10,10 @@
};
use ruff_text_size::{Ranged, TextRange, TextSlice};
-use crate::QuoteStyle;
use crate::context::InterpolatedStringState;
use crate::prelude::*;
use crate::string::{Quote, StringQuotes, TripleQuotes};
+use crate::{FStringConsistentQuotes, QuoteStyle};
pub(crate) struct StringNormalizer<'a, 'src> {
preferred_quote_style: Option<QuoteStyle>,
@@ -68,18 +68,25 @@
.is_enabled();
if !parent_flags.is_triple_quoted() || string.flags().is_triple_quoted() {
- // When f_string_consistent_quotes is enabled AND we're targeting Python 3.12+,
- // use the preferred quote style consistently.
- if supports_pep_701 && consistent_quotes && !preferred_quote_style.is_preserve() {
- return preferred_quote_style;
- }
- // Otherwise, use alternating quotes for compatibility.
// This logic is even necessary when using preserve and the target python version doesn't support PEP701 because
// we might end up joining two f-strings that have different quote styles, in which case we need to alternate the quotes
// for inner strings to avoid a syntax error: `string = "this is my string with " f'"{params.get("mine")}"'`
- if !preferred_quote_style.is_preserve() || !supports_pep_701 {
+ if !supports_pep_701 {
return QuoteStyle::from(parent_flags.quote_style().opposite());
}
+
+ if !preferred_quote_style.is_preserve() {
+ match self.context.options().f_string_consistent_quotes() {
+ FStringConsistentQuotes::Disabled => {
+ return QuoteStyle::from(parent_flags.quote_style().opposite());
+ }
+ // When f_string_consistent_quotes is enabled AND we're targeting Python 3.12+,
+ // use the preferred quote style consistently.
+ FStringConsistentQuotes::Enabled => {
+ return QuoteStyle::from(parent_flags.quote_style());
+ }
+ }
+ }
}
}
| @@ -1,5 +1,6 @@ | |||
| --- | |||
| source: crates/ruff_python_formatter/tests/fixtures.rs | |||
| assertion_line: 267 | |||
There was a problem hiding this comment.
Can you try upgrading your cargo insta version? I believe cargo insta stopped adding assertion line metadata to snapshot but I'm not a 100% sure. If so, please revert the assertion_line changes.
| quoted_in_expr = f"Result: {get_value(key='test', default='none')}" | ||
|
|
||
| # Multiple f-strings in one expression | ||
| multiple = f"First: {x}" + f"Second: {y}" |
There was a problem hiding this comment.
Let's also add some tests for t-strings
? |
|
Having thought about this more, I think there's one remaining design question. To me it's unclear whether I feel like we want the preferred quotes over strictly enforcing consistency. If it's consistency that we want, then I think we'd have to change the quote selection no not only make a local selection, but pick the quote style that results in the fewest quotes for the entire f-string (seems unnecessary complicated to me) |
|
Hi Micha, thanks for the review. Let's resolve the behavior question first, then naming and code nits. I think there are two clean choices: A: Always use preferred quotes for nested strings rather than strictly enforcing consistency with the outermost quotes. Given the existing escape minimization behavior and the potential complexity of optimizing escapes in nested strings too, I agree with you this seems the simplest and most logical choice. B: Add an option to always use preferred quotes for all strings - outermost and nested - disabling escape minimization entirely. This would subsume A. For example, it could be named Rationales for B:
While I actually prefer B, A is fine if you prefer to keep the scope small. |
|
I'm not sure I correctly understand what you propose in option The options I suggested are: A: The same as your option A. Each f-string (including nested) always pick their preferred quote style if the new setting introduced in this PR is true instead of alternating. The quote selection tries to minimize escapes only in the current f-string, disregarding any escapes in nested f-strings. In most cases, this is the same as always using the same quotes. B: Always use the quotes from the outer f-string but change the quote-style selection to select the quote style to also consider nested f-string so that the selected quote style requires the minimal escapes across all nested f-strings (and not just the outermost). I'm leaning towards option A because:
|
OK, yes it would be. I will open a separate Feature Request after this is merged to gauge support for that simplifying proposal.
Great, we are agreed on this. Next, naming. Updating your earlier suggestions to reflect preferred instead of consistent:
Let's eliminate (2) and (4): the options apply only to nested interpolated strings rather than all interpolated strings as the names suggest (many users might think In (3), "template" suggests t-strings, but this will also apply to f-strings. That leaves:
Are there any other kinds of nested string other than interpolated ones, for the purposes of the formatter? Presumably strings inside strings like "'is this nested'" don't count since they aren't relevant to the formatter. If not, we can simplify this to just:
This is similar to @zsol's suggestion, which we should update since escape minimization could mean our option (A) won't be "uniform", to:
I think |
If we think this is what users want, I would rather know before merging this PR. I don't think we should support three options here.
I prefer
This is my preferred spelling. I'm not too opinionated on |
OK, I've requested some feedback here: #14118 (comment). Let's put this PR on hold while waiting for that.
Agreed,
|
|
Replaced by a new PR implementing the option variant we chose: #24312 |
Introduces a new formatter option
f-string-consistent-quotesthat leverages Python 3.12's PEP 701 to use consistent quotes inside f-string expressions rather than alternating quote styles for compatibility.When enabled and targeting Python 3.12+, the formatter will use the same quote style (following the
quote-stylesetting) inside f-string expressions as in the outer f-string. This produces more consistent and readable code.When disabled (default) or targeting Python versions below 3.12, the formatter will continue to alternate quotes for compatibility.
Implements: #14118
Original PR (closed): #16385