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.

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 f-string formatting in assignment statement #14454

Open
wants to merge 6 commits into
base: dhruv/f-string-assignment-1
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,94 @@
# comment 28
} woah {x}"

# Assignment statement

# Even though this f-string has multiline expression, thus allowing us to break it at the
# curly braces, the f-string fits on a single line if it's moved inside the parentheses.
# We should prefer doing that instead.
aaaaaaaaaaaaaaaaaa = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
expression}moreeeeeeeeeeeeeeeee"

# Same as above
xxxxxxx = f"{
{'aaaaaaaaaaaaaaaaaaa', 'bbbbbbbbbbbbbbbbbbb', 'cccccccccccccccccccccccccc'}
}"

# Similar to the previous example, but the f-string will exceed the line length limit,
# we shouldn't add any parentheses here.
xxxxxxx = f"{
{'aaaaaaaaaaaaaaaaaaaaaaaaa', 'bbbbbbbbbbbbbbbbbbbbbbbbbbb', 'cccccccccccccccccccccccccc'}
}"

# Same as above but with an inline comment. The f-string should be formatted inside the
# parentheses and the comment should be part of the line inside the parentheses.
aaaaaaaaaaaaaaaaaa = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
expression}moreeeeeeeeeeeeeeeee" # comment

# Similar to the previous example but this time parenthesizing doesn't work because it
# exceeds the line length. So, avoid parenthesizing this f-string.
aaaaaaaaaaaaaaaaaa = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
expression}moreeeeeeeeeeeeeeeee" # comment loooooooong

# Similar to the previous example but we start with the parenthesized layout. This should
# remove the parentheses and format the f-string on a single line. This shows that the
# final layout for the formatter is same for this and the previous case. The only
# difference is that in the previous case the expression is already mulitline which means
# the formatter can break it further at the curly braces.
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{expression}moreeeeeeeeeeeeeeeee" # comment loooooooong
)

# The following f-strings are going to break because of the trailing comma so we should
# avoid using the best fit layout and instead use the default layout.
# left-to-right
aaaa = f"aaaa {[
1, 2,
]} bbbb"
# right-to-left
aaaa, bbbb = f"aaaa {[
1, 2,
]} bbbb"

# Using the right-to-left assignment statement variant.
aaaaaaaaaaaaaaaaaa, bbbbbbbbbbb = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
expression}moreeeeeeeeeeeeeeeee" # comment

# Here, the f-string layout is flat but it exceeds the line length limit. This shouldn't
# try the custom best fit layout because the f-string doesn't have any split points.
aaaaaaaaaaaa["bbbbbbbbbbbbbbbb"] = (
f"aaaaaaaaaaaaaaaaaaa {aaaaaaaaa + bbbbbbbbbbb + cccccccccccccc} ddddddddddddddddddd"
)
# Same as above but without the parentheses to test that it gets formatted to the same
# layout as the previous example.
aaaaaaaaaaaa["bbbbbbbbbbbbbbbb"] = f"aaaaaaaaaaaaaaaaaaa {aaaaaaaaa + bbbbbbbbbbb + cccccccccccccc} ddddddddddddddddddd"

# But, the following f-string does have a split point because of the multiline expression.
aaaaaaaaaaaa["bbbbbbbbbbbbbbbb"] = (
f"aaaaaaaaaaaaaaaaaaa {
aaaaaaaaa + bbbbbbbbbbb + cccccccccccccc} ddddddddddddddddddd"
)
aaaaaaaaaaaa["bbbbbbbbbbbbbbbb"] = (
f"aaaaaaaaaaaaaaaaaaa {
aaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbb + cccccccccccccccccccccc + dddddddddddddddddddddddddddd} ddddddddddddddddddd"
)
Comment on lines +383 to +386
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some examples with inner comments or debug expressions (and format specs)? See

# Don't inline f-strings that contain expressions that are guaranteed to split, e.b. because of a magic trailing comma
aaaaaaaaaaaaaaaaaa = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
[a,]
}" "moreeeeeeeeeeeeeeeeeeee" "test" # comment
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
[a,]
}" "moreeeeeeeeeeeeeeeeeeee" "test" # comment
)
aaaaa[aaaaaaaaaaa] = f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
[a,]
}" "moreeeeeeeeeeeeeeeeeeee" "test" # comment
aaaaa[aaaaaaaaaaa] = (f"testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee{
[a,]
}" "moreeeeeeeeeeeeeeeeeeee" "test" # comment
)
# Don't inline f-strings that contain commented expressions
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{[
a # comment
]}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
aaaaa[aaaaaaaaaaa] = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{[
a # comment
]}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
# Don't inline f-strings with multiline debug expressions:
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{
a=}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{a +
b=}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
aaaaaaaaaaaaaaaaaa = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{a
=}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
aaaaa[aaaaaaaaaaa] = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{
a=}" "moreeeeeeeeeeeeeeeeeetest" # comment
)
aaaaa[aaaaaaaaaaa] = (
f"testeeeeeeeeeeeeeeeeeeeeeeeee{a
=}" "moreeeeeeeeeeeeeeeeeetest" # comment
)


# This is an implicitly concatenated f-string but it cannot be joined because otherwise
# it'll exceed the line length limit. So, the two f-strings will be inside parentheses
# instead and the inline comment should be outside the parentheses.
a = f"test{
expression
}flat" f"can be {
joined
} togethereeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" # inline

# Similar to the above example but this fits within the line length limit.
a = f"test{
expression
}flat" f"can be {
joined
} togethereeeeeeeeeeeeeeeeeeeeeeeeeee" # inline

# Indentation

# What should be the indentation?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ impl NeedsParentheses for ExprBinOp {
} else if let Ok(string) = StringLike::try_from(&*self.left) {
// Multiline strings are guaranteed to never fit, avoid adding unnecessary parentheses
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an example covering this path with an expression that starts as a mulitline f-string but then gets formatted as a flat f-string?

if !string.is_implicit_concatenated()
&& string.is_multiline(context.source())
&& string.is_multiline(context)
&& has_parentheses(&self.right, context).is_some()
&& !context.comments().has_dangling(self)
&& !context.comments().has(string)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl NeedsParentheses for ExprBytesLiteral {
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
} else if StringLike::Bytes(self).is_multiline(context.source()) {
} else if StringLike::Bytes(self).is_multiline(context) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl NeedsParentheses for ExprCompare {
} else if let Ok(string) = StringLike::try_from(&*self.left) {
// Multiline strings are guaranteed to never fit, avoid adding unnecessary parentheses
if !string.is_implicit_concatenated()
&& string.is_multiline(context.source())
&& string.is_multiline(context)
&& !context.comments().has(string)
&& self.comparators.first().is_some_and(|right| {
has_parentheses(right, context).is_some() && !context.comments().has(right)
Expand Down
27 changes: 5 additions & 22 deletions crates/ruff_python_formatter/src/expression/expr_f_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ use crate::expression::parentheses::{
};
use crate::other::f_string::FormatFString;
use crate::prelude::*;
use crate::string::implicit::FormatImplicitConcatenatedStringFlat;
use crate::string::{implicit::FormatImplicitConcatenatedString, Quoting, StringLikeExtensions};
use crate::string::implicit::{
FormatImplicitConcatenatedString, FormatImplicitConcatenatedStringFlat,
};
use crate::string::{Quoting, StringLikeExtensions};

#[derive(Default)]
pub struct FormatExprFString;
Expand Down Expand Up @@ -45,26 +47,7 @@ impl NeedsParentheses for ExprFString {
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
}
// TODO(dhruvmanila): Ideally what we want here is a new variant which
// is something like:
// - If the expression fits by just adding the parentheses, then add them and
// avoid breaking the f-string expression. So,
// ```
// xxxxxxxxx = (
// f"aaaaaaaaaaaa { xxxxxxx + yyyyyyyy } bbbbbbbbbbbbb"
// )
// ```
// - But, if the expression is too long to fit even with parentheses, then
// don't add the parentheses and instead break the expression at `soft_line_break`.
// ```
// xxxxxxxxx = f"aaaaaaaaaaaa {
// xxxxxxxxx + yyyyyyyyyy
// } bbbbbbbbbbbbb"
// ```
// This isn't decided yet, refer to the relevant discussion:
// https://github.com/astral-sh/ruff/discussions/9785
else if StringLike::FString(self).is_multiline(context.source()) {
} else if StringLike::FString(self).is_multiline(context) {
Copy link
Member

Choose a reason for hiding this comment

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

I think using BestFit for all f-strings that contain multiline expressions isn't correct. For example. It results in

if f"aaaaaaaaaaa { ttttteeeeeeeeest} more {
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
}": pass

being formatted as

if f"aaaaaaaaaaa {ttttteeeeeeeeest} more {aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa}":
    pass

Which seems worse than before.

I think we should keep using Multiline if the f-string has any multiline expression to avoid collapsing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the reason it was formatted like that previously is because the needs_parentheses would return OptionalParentheses::Never. If we would return Multiline then I think parentheses will be added making it:

if (
    f"aaaaaaaaaaa {ttttteeeeeeeeest} more {
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    }"
):
    pass

Copy link
Member

Choose a reason for hiding this comment

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

We can also decide to return Never. It's not entirely clear to me what the better and more consistent layout is. I would have to play with a few layouts but I don't think it should be what it is now.

Using Never has the advantage that it avoids unnecessary parentheses and is closer to what we had today (and no one complained?). Adding parentheses is similar to having "aaaaa" + tttttt + "more(aaaaaaaaaaaaaaaaaaaaaaaaaa). It might be good to play with the formatting in other positions where we use BestFit to decide what the best layout is

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea, I can do that.

OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl NeedsParentheses for ExprStringLiteral {
) -> OptionalParentheses {
if self.value.is_implicit_concatenated() {
OptionalParentheses::Multiline
} else if StringLike::String(self).is_multiline(context.source()) {
} else if StringLike::String(self).is_multiline(context) {
OptionalParentheses::Never
} else {
OptionalParentheses::BestFit
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_python_formatter/src/other/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ fn is_huggable_string_argument(
arguments: &Arguments,
context: &PyFormatContext,
) -> bool {
if string.is_implicit_concatenated() || !string.is_multiline(context.source()) {
if string.is_implicit_concatenated() || !string.is_multiline(context) {
return false;
}
Comment on lines +226 to 228
Copy link
Member

Choose a reason for hiding this comment

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

Your change is an improvement to what we had before.

For example, this was correctly formatted code before

call(f"{
    testeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
}")

But I don't think it's correct to use the "hug" layout if an inner expression is multiline

call(f"{
    aaaaaa
    + '''test
    more'''
}")

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow here. Are you saying that both the code snippet should result in the formatting where the f-string is on the same line as the call expression (call(f"{)?

Copy link
Member

Choose a reason for hiding this comment

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

This is not directly related to your PR, but I noticed it when reviewing it because you changed is_multiline. We have to make a decision on the idiomatic formatting for "hugging multiline strings" for f-strings.

I'm leaning towards that we should only "hug" if the f-string itself contains any multiline string literal, but not if any inner-expression is multiline. Similar to prettier:

call(
  `${
    aaaaaaaaaaaaaaaaaaaaaaaaaaaa +
    `test more
    aaa` +
    morrrrrrrrrrrrrrrrrrrrrr
  }`,
);

So what I'm saying is that we should probably not hug for the both cases above.


Expand Down
4 changes: 4 additions & 0 deletions crates/ruff_python_formatter/src/other/f_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ impl FStringLayout {
}
}

pub(crate) const fn is_flat(self) -> bool {
matches!(self, FStringLayout::Flat)
}

pub(crate) const fn is_multiline(self) -> bool {
matches!(self, FStringLayout::Multiline)
}
Expand Down
Loading
Loading