-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Preview minimal f-string formatting #9642
Conversation
1f2328a
to
c87cd15
Compare
c1e8f9d
to
870c2a6
Compare
55195fd
to
8627f40
Compare
4216d2e
to
f839da3
Compare
8627f40
to
d3402d7
Compare
b9e26ee
to
5274d78
Compare
|
77830be
to
7b2a6b2
Compare
a431f6a
to
334616d
Compare
7b2a6b2
to
c80ab7f
Compare
c80ab7f
to
18b88c4
Compare
CodSpeed Performance ReportMerging #9642 will not alter performanceComparing Summary
|
// } bbbbbbbbbbbbb" | ||
// ``` | ||
// This isn't decided yet, refer to the relevant discussion: | ||
// https://github.com/astral-sh/ruff/discussions/9785 | ||
} else if AnyString::FString(self).is_multiline(context.source()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to change this to only return true
when the string literals are multiline or we risk instability if an f-string becomes single line because we collapse the expression part. Or is this handled somewhere else?
3d31f69
to
4c62227
Compare
I've some understand questions first:
Is the logic if replacement expressions are allowed to expand over multiple lines globally for the entire f-string-literal (expand if any replacement expression contains a line break), or is the decision made for each replacement expression (only expand the replacement expressions that already contain line breaks, keep the other ones flat)?
Does this also apply to Python 3.12 or does Python 3.12 allow to reuse the same triple quotes?
Does that mean we do not format any f-string that contains any debug expression or is it that we only disable formatting for replacements that contain debug expressions? |
The decision is made globally for an entire f-string by looking at each replacement field and checking if any of them contains a line break.
Python 3.12 allows re-use of same quotes irrespective of whether it's single or triple quoted. The logic basically ORs with checking if the target-version is 3.12 or not.
The later. So, given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @dhruvmanila
We can follow up with the indent handling in a separate PR if we decide to change it.
I have a few comments about the implementations; most are small Nits.
My main concerns are about combining the RemoveSoftlinesBuffer
with the Printer
approach and how trailing comments are now associated as dangling comments.
I suggest that we either use the RemoveSoftlinesBuffer
or the Printer
approach but that we avoid using both to keep things "simple". If you prefer to keep RemoveSoftlinesBuffer
, then I would prefer changing the magic trailing comma handling by checking some state in the context over using the Printer
. It avoids the performance penalty and doesn't require a visitor to determine if it is necessary to use it or not.
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py
Outdated
Show resolved
Hide resolved
Thanks for the detailed review @MichaReiser, really appreciate all the suggestions you've provided. Regarding the magic trailing comma, I've removed the ruff/crates/ruff_python_formatter/src/builders.rs Lines 207 to 215 in 5c056a1
I'll look at the final ecosystem changes and then merge this PR. |
For posterity... Currently, to avoid breaking the expression, what we do is after the expression formatting is over, we'd remove the line breaks. This creates a problem w.r.t. the magic trailing comma. For example, f"aaaaaaa {['aaaaaaaaaaaaaaa', 'bbbbbbbbbbbbb', 'ccccccccccccccccc', 'ddddddddddddddd', 'eeeeeeeeeeeeee']} aaaaaaa" The formatter will break the list but as there were no line breaks in the original source code, we'd collapse it back. The trailing comma will still remain: f"aaaaaaa {['aaaaaaaaaaaaaaa', 'bbbbbbbbbbbbb', 'ccccccccccccccccc', 'ddddddddddddddd', 'eeeeeeeeeeeeee',]} aaaaaaa" The current approach to solving this problem is to update the builder to not add the trailing comma in this specific context: ruff/crates/ruff_python_formatter/src/builders.rs Lines 207 to 215 in 5c056a1
An alternative approach, which is what this comment documents, is to use the
This also has an advantage that the logic is local to the f-string formatting. |
## Summary _This is preview only feature and is available using the `--preview` command-line flag._ With the implementation of [PEP 701] in Python 3.12, f-strings can now be broken into multiple lines, can contain comments, and can re-use the same quote character. Currently, no other Python formatter formats the f-strings so there's some discussion which needs to happen in defining the style used for f-string formatting. Relevant discussion: astral-sh#9785 The goal for this PR is to add minimal support for f-string formatting. This would be to format expression within the replacement field without introducing any major style changes. ### Newlines The heuristics for adding newline is similar to that of [Prettier](https://prettier.io/docs/en/next/rationale.html#template-literals) where the formatter would only split an expression in the replacement field across multiple lines if there was already a line break within the replacement field. In other words, the formatter would not add any newlines unless they were already present i.e., they were added by the user. This makes breaking any expression inside an f-string optional and in control of the user. For example, ```python # We wouldn't break this aaaaaaaaaaa = f"asaaaaaaaaaaaaaaaa { aaaaaaaaaaaa + bbbbbbbbbbbb + ccccccccccccccc } cccccccccc" # But, we would break the following as there's already a newline aaaaaaaaaaa = f"asaaaaaaaaaaaaaaaa { aaaaaaaaaaaa + bbbbbbbbbbbb + ccccccccccccccc } cccccccccc" ``` If there are comments in any of the replacement field of the f-string, then it will always be a multi-line f-string in which case the formatter would prefer to break expressions i.e., introduce newlines. For example, ```python x = f"{ # comment a }" ``` ### Quotes The logic for formatting quotes remains unchanged. The existing logic is used to determine the necessary quote char and is used accordingly. Now, if the expression inside an f-string is itself a string like, then we need to make sure to preserve the existing quote and not change it to the preferred quote unless it's 3.12. For example, ```python f"outer {'inner'} outer" # For pre 3.12, preserve the single quote f"outer {'inner'} outer" # While for 3.12 and later, the quotes can be changed f"outer {"inner"} outer" ``` But, for triple-quoted strings, we can re-use the same quote char unless the inner string is itself a triple-quoted string. ```python f"""outer {"inner"} outer""" # valid f"""outer {'''inner'''} outer""" # preserve the single quote char for the inner string ``` ### Debug expressions If debug expressions are present in the replacement field of a f-string, then the whitespace needs to be preserved as they will be rendered as it is (for example, `f"{ x = }"`. If there are any nested f-strings, then the whitespace in them needs to be preserved as well which means that we'll stop formatting the f-string as soon as we encounter a debug expression. ```python f"outer { x = !s :.3f}" # ^^ # We can remove these whitespaces ``` Now, the whitespace doesn't need to be preserved around conversion spec and format specifiers, so we'll format them as usual but we won't be formatting any nested f-string within the format specifier. ### Miscellaneous - The [`hug_parens_with_braces_and_square_brackets`](astral-sh#8279) preview style isn't implemented w.r.t. the f-string curly braces. - The [indentation](astral-sh#9785 (comment)) is always relative to the f-string containing statement ## Test Plan * Add new test cases * Review existing snapshot changes * Review the ecosystem changes [PEP 701]: https://peps.python.org/pep-0701/
Summary
This is preview only feature and is available using the
--preview
command-line flag.With the implementation of PEP 701 in Python 3.12, f-strings can now be broken into multiple lines, can contain comments, and can re-use the same quote character. Currently, no other Python formatter formats the f-strings so there's some discussion which needs to happen in defining the style used for f-string formatting. Relevant discussion: #9785
The goal for this PR is to add minimal support for f-string formatting. This would be to format expression within the replacement field without introducing any major style changes.
Newlines
The heuristics for adding newline is similar to that of Prettier where the formatter would only split an expression in the replacement field across multiple lines if there was already a line break within the replacement field.
In other words, the formatter would not add any newlines unless they were already present i.e., they were added by the user. This makes breaking any expression inside an f-string optional and in control of the user. For example,
If there are comments in any of the replacement field of the f-string, then it will always be a multi-line f-string in which case the formatter would prefer to break expressions i.e., introduce newlines. For example,
Quotes
The logic for formatting quotes remains unchanged. The existing logic is used to determine the necessary quote char and is used accordingly.
Now, if the expression inside an f-string is itself a string like, then we need to make sure to preserve the existing quote and not change it to the preferred quote unless it's 3.12. For example,
But, for triple-quoted strings, we can re-use the same quote char unless the inner string is itself a triple-quoted string.
Debug expressions
If debug expressions are present in the replacement field of a f-string, then the whitespace needs to be preserved as they will be rendered as it is (for example,
f"{ x = }"
. If there are any nested f-strings, then the whitespace in them needs to be preserved as well which means that we'll stop formatting the f-string as soon as we encounter a debug expression.Now, the whitespace doesn't need to be preserved around conversion spec and format specifiers, so we'll format them as usual but we won't be formatting any nested f-string within the format specifier.
Miscellaneous
hug_parens_with_braces_and_square_brackets
preview style isn't implemented w.r.t. the f-string curly braces.Test Plan