-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Hug closing }
when f-string expression has a format specifier
#18704
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
Conversation
@@ -304,14 +304,14 @@ | |||
""" | |||
|
|||
# Mix of various features. | |||
f"{ # comment 26 | |||
f"""{ # comment 26 |
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.
This example is a syntax error for a single quoted string, but it's perfectly fine for a triple quoted string
|
4e8bbad
to
5056b8c
Compare
5056b8c
to
be72aa7
Compare
}
when f-string expression has a format specifier
|
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.
This makes sense to me, for what it's worth. It definitely makes sense to wait for Dhruv's review, just wanted to take a quick look too in case it helped.
if conversion.is_none() && format_spec.is_none() { | ||
bracket_spacing.fmt(f)?; | ||
} else if comments.has_trailing_own_line(self.element) { |
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.
Is this empty branch part of the TODO above?
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.
Lol no, that's just me moving stuff around and forgetting to delete it (and clippy not telling me)
@ntBre for the changelog (assuming this makes it in). We could probably write something like this Ruff now formats multiline f-strings with format-specifiers differently because of a change to CPython's grammar. Before, Ruff formatted the interpolation expression like so: f"This is some long string {
x:d
} that is formatted across multiple lines" The line break after the f"This is some long string {
x:d} that is formatted across multiple lines" |
Thanks! I'll add it to the blog post too. There's no real rush on getting the release out from my side, so I think we can wait until this is ready. |
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.
Looks great! Thank you for the quick fix!
crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py
Outdated
Show resolved
Hide resolved
// TODO: The `or comments.has_trailing...` can be removed once newlines in format specs are a syntax error. | ||
// This is to support the following case: | ||
// ```py | ||
// x = f"{x !s | ||
// :>0 | ||
// # comment 21 | ||
// }" | ||
// ``` | ||
if format_spec.is_none() || comments.has_trailing_own_line(self.element) { |
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'm guessing the example mentioned in the comment is specifically to target the format_spec.is_none
case and not the trailing comment case. And, as the latter case is going to be removed, we don't really need to provide an example for that here.
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.
The example is for the format_spec.is_some
case. It will be removed in my next PR.
I mainly provided it because I found it helpful to reason about all the changes i've been making. I think I keep it, considering that I already have a PR that removes it again.
Yeah, I think I mainly did this because the newline for a triple-quoted f-string is a newline character in the literal string and not an actual newline, so that didn't account for the fact that the f-string is multiline. For example, the following is not a multiline f-string: aaaaaaaaaaa = f"""asaaaaaaaaaaaaaaaa {aaaaaaaaaaaa + bbbbbbbbbbbb + ccccccccccccccc + dddddddd:.3f
} cccccccccc""" But, it does create inconsistent behavior between triple-quoted and single-quoted f-string which you've noticed. One might argue that is not really an inconsistent behavior as it does follow what we stated regarding when the expression could be broken down i.e., only when the user already has a multiline f-string. In reality, I guess this shouldn't affect too many users as newlines in format spec should be rare enough especially now that it's a syntax error. So, I'm fine with this change. |
I learned this the hard way when working on this PR 😅. Thanks for adding tests for it.
Yeah, that was my thinking too and seems to be confirmed by our ecosyste checks. |
@ntBre I'll merge this to main, considering that we plan to do the minor release today. |
Summary
This PR changes how we format f-string expressions with a format specifier.
Before:
}
. This is now invalid syntax under Python 3.13.4.This PR changes two address the invalid syntax and make single and triple quoted strings more consisent:
f- or t-string elements with a format specifier now hug the closing
}
. This ensures that the formatter doesn't introduce a now invalid line break after the format specifier.Remove the restriction that triple-quoted f- or t-strings can't use the multiline layout if they have a format specifier. This isn't required to fix the invalid syntax BUT it fixes an issue where the formatter requires two passes to correctly format comments nested inside an interpolated format specifier:
which before this PR gets formatted to:
which is fairly different from the source. I think making this change should be fine, if this PR makes it into the minor release tomorrow.
I considered if we should always keep elements with a format specifier flat. However, this gets very tricky if the format specifier is already formatted over multiple lines and contains comments (see the triple-quoted case above). That's why I opted to allow them and making the behavior consistent between single and triple quoted strings.
Fixes #18672
Fixes #18632
Note, this PR doesn't change the parser / lexer
Test Plan
Updated / added tests