Skip to content

Commit

Permalink
Fix unstable f-string formatting for expressions containing a trailin…
Browse files Browse the repository at this point in the history
…g comma (#15545)
  • Loading branch information
MichaReiser authored Jan 17, 2025
1 parent fdb9f4e commit 1ecb7ce
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -719,10 +719,15 @@
print(f"{1, 2, {3} }")
print(f"{(1, 2, {3})}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15535
print(f"{ {}, }") # A single item tuple gets parenthesized
print(f"{ {}.values(), }")
print(f"{ {}, 1 }") # A tuple with multiple elements doesn't get parenthesized
print(f"{ # Tuple with multiple elements that doesn't fit on a single line gets parenthesized
{}, 1,
}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15536
print(f"{ {}, 1, }")
14 changes: 14 additions & 0 deletions crates/ruff_python_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,20 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> {

pub(crate) fn finish(&mut self) -> FormatResult<()> {
self.result.and_then(|()| {
// Don't add a magic trailing comma when formatting an f-string expression
// that always must be flat because the `expand_parent` forces enclosing
// groups to expand, e.g. `print(f"{(a,)} ")` would format the f-string in
// flat mode but the `print` call gets expanded because of the `expand_parent`.
if self
.fmt
.context()
.f_string_state()
.can_contain_line_breaks()
== Some(false)
{
return Ok(());
}

if let Some(last_end) = self.entries.position() {
let magic_trailing_comma = has_magic_trailing_comma(
TextRange::new(last_end, self.sequence_end),
Expand Down
11 changes: 11 additions & 0 deletions crates/ruff_python_formatter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,17 @@ pub(crate) enum FStringState {
Outside,
}

impl FStringState {
pub(crate) fn can_contain_line_breaks(self) -> Option<bool> {
match self {
FStringState::InsideExpressionElement(context) => {
Some(context.can_contain_line_breaks())
}
FStringState::Outside => None,
}
}
}

/// The position of a top-level statement in the module.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Default)]
pub(crate) enum TopLevelStatementPosition {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -725,13 +725,18 @@ print(f"{({1, 2, 3}) - ({2})}")
print(f"{1, 2, {3} }")
print(f"{(1, 2, {3})}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15535
print(f"{ {}, }") # A single item tuple gets parenthesized
print(f"{ {}.values(), }")
print(f"{ {}, 1 }") # A tuple with multiple elements doesn't get parenthesized
print(f"{ # Tuple with multiple elements that doesn't fit on a single line gets parenthesized
{}, 1,
}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15536
print(f"{ {}, 1, }")
```
## Outputs
Expand Down Expand Up @@ -1515,6 +1520,7 @@ print(f"{({1, 2, 3}) - ({2})}")
print(f"{1, 2, {3}}")
print(f"{(1, 2, {3})}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15535
print(f"{({},)}") # A single item tuple gets parenthesized
print(f"{({}.values(),)}")
Expand All @@ -1527,6 +1533,10 @@ print(
)
}"
)


# Regression tests for https://github.com/astral-sh/ruff/issues/15536
print(f"{ {}, 1 }")
```
Expand Down Expand Up @@ -2310,6 +2320,7 @@ print(f"{({1, 2, 3}) - ({2})}")
print(f"{1, 2, {3}}")
print(f"{(1, 2, {3})}")


# Regression tests for https://github.com/astral-sh/ruff/issues/15535
print(f"{({},)}") # A single item tuple gets parenthesized
print(f"{({}.values(),)}")
Expand All @@ -2322,4 +2333,8 @@ print(
)
}"
)


# Regression tests for https://github.com/astral-sh/ruff/issues/15536
print(f"{ {}, 1 }")
```

0 comments on commit 1ecb7ce

Please sign in to comment.