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

Fix joining of f-strings with different quotes when using quote style Preserve #15524

Merged
merged 2 commits into from
Jan 16, 2025
+55 −11
Merged

Fix joining of f-strings with different quotes when using quote style Preserve #15524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9c9aab
Fix joining of f-strings with different quotes when using quote style…
MichaReiser Jan 16, 2025
701e96d
Discard changes to crates/ruff_python_formatter/src/lib.rs
MichaReiser Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...ter/resources/test/fixtures/ruff/expression/join_implicit_concatenated_string_preserve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,12 @@

# Already invalid Pre Python 312
f"{'Hy "User"'}" f'{"Hy 'User'"}'


# Regression tests for https://github.com/astral-sh/ruff/issues/15514
params = {}
string = "this is my string with " f'"{params.get("mine")}"'
string = f'"{params.get("mine")} ' f"with {'nested single quoted string'}"
string = f"{'''inner ' '''}" f'{"""inner " """}'
string = f"{10 + len('bar')=}" f"{10 + len('bar')=}"
string = f"{10 + len('bar')=}" f'{10 + len("bar")=}'
Comment on lines +16 to +22
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add some test cases which includes format spec.

Is the following correct?

 params = {}
-string = "this is my string with " f'"{params.get("mine")=:{"xxx"}}"'
+string = f'this is my string with "{params.get("mine")=:{"xxx"}}"'

with quote-style preserve and < 3.12 Python.

Or, is there no way to preserve the quote style here?

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be relevant to this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no strictly correct answer when it comes to preserve and joining implicit concatenated strings if not all parts use the same quotes. You have to choose which quotes you want to preserve and in this example, the formatter decides to preserve the quotes from the second part because the string can't be joined otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable, thanks

30 changes: 19 additions & 11 deletions crates/ruff_python_formatter/src/string/normalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,33 @@ impl<'a, 'src> StringNormalizer<'a, 'src> {
let preferred_quote_style = self
.preferred_quote_style
.unwrap_or(self.context.options().quote_style());
let supports_pep_701 = self.context.options().target_version().supports_pep_701();

// For f-strings prefer alternating the quotes unless The outer string is triple quoted and the inner isn't.
if let FStringState::InsideExpressionElement(parent_context) = self.context.f_string_state()
{
let parent_flags = parent_context.f_string().flags();

if !parent_flags.is_triple_quoted() || string.flags().is_triple_quoted() {
// 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 {
return QuoteStyle::from(parent_flags.quote_style().opposite());
}
}
}

// Leave the quotes unchanged for all other strings.
if preferred_quote_style.is_preserve() {
return QuoteStyle::Preserve;
}

// There are cases where it is necessary to preserve the quotes to prevent an invalid f-string.
if let StringLikePart::FString(fstring) = string {
// There are two cases where it's necessary to preserve the quotes if the
// target version is pre 3.12 and the part is an f-string.
if !self.context.options().target_version().supports_pep_701() {
if !supports_pep_701 {
// An f-string expression contains a debug text with a quote character
// because the formatter will emit the debug expression **exactly** the
// same as in the source text.
Expand All @@ -77,16 +95,6 @@ impl<'a, 'src> StringNormalizer<'a, 'src> {
}
}

// For f-strings prefer alternating the quotes unless The outer string is triple quoted and the inner isn't.
if let FStringState::InsideExpressionElement(parent_context) = self.context.f_string_state()
{
let parent_flags = parent_context.f_string().flags();

if !parent_flags.is_triple_quoted() || string.flags().is_triple_quoted() {
return QuoteStyle::from(parent_flags.quote_style().opposite());
}
}

// Per PEP 8, always prefer double quotes for triple-quoted strings.
if string.flags().is_triple_quoted() {
// ... unless we're formatting a code snippet inside a docstring,
Expand Down
27 changes: 27 additions & 0 deletions ...ter/tests/snapshots/format@expression__join_implicit_concatenated_string_preserve.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ a = "different '" 'quote "are fine"' # join

# Already invalid Pre Python 312
f"{'Hy "User"'}" f'{"Hy 'User'"}'


# Regression tests for https://github.com/astral-sh/ruff/issues/15514
params = {}
string = "this is my string with " f'"{params.get("mine")}"'
string = f'"{params.get("mine")} ' f"with {'nested single quoted string'}"
string = f"{'''inner ' '''}" f'{"""inner " """}'
string = f"{10 + len('bar')=}" f"{10 + len('bar')=}"
string = f"{10 + len('bar')=}" f'{10 + len("bar")=}'
```

## Outputs
Expand Down Expand Up @@ -49,6 +58,15 @@ a = "different 'quote \"are fine\"" # join

# Already invalid Pre Python 312
f"{'Hy "User"'}{"Hy 'User'"}"


# Regression tests for https://github.com/astral-sh/ruff/issues/15514
params = {}
string = f"this is my string with \"{params.get('mine')}\""
string = f'"{params.get("mine")} with {"nested single quoted string"}'
string = f"{'''inner ' '''}" f'{"""inner " """}'
string = f"{10 + len('bar')=}{10 + len('bar')=}"
string = f"{10 + len('bar')=}" f'{10 + len("bar")=}'
```


Expand Down Expand Up @@ -81,4 +99,13 @@ a = "different 'quote \"are fine\"" # join

# Already invalid Pre Python 312
f"{'Hy "User"'}{"Hy 'User'"}"


# Regression tests for https://github.com/astral-sh/ruff/issues/15514
params = {}
string = f"this is my string with \"{params.get("mine")}\""
string = f'"{params.get("mine")} with {'nested single quoted string'}'
string = f"{'''inner ' '''}{"""inner " """}"
string = f"{10 + len('bar')=}{10 + len('bar')=}"
string = f"{10 + len('bar')=}{10 + len("bar")=}"
```
Loading