From ccac9681e1d02f6c406ed03531f299226ac1a01f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 22 Aug 2023 12:27:20 +0200 Subject: [PATCH] Preserve yield parentheses (#6766) --- .../test/fixtures/ruff/expression/yield.py | 32 ++++++++ .../src/expression/expr_await.rs | 2 +- .../src/expression/expr_yield.rs | 2 +- .../src/expression/mod.rs | 17 ++-- .../src/expression/parentheses.rs | 5 +- .../format@expression__yield.py.snap | 79 ++++++++++++++++++- 6 files changed, 121 insertions(+), 16 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py index 2c5e07c6f7..ff587bd812 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/yield.py @@ -65,3 +65,35 @@ def foo(): test, # comment 4 1 ) + +yield ("Cache key will cause errors if used with memcached: %r " "(longer than %s)" % ( + key, + MEMCACHE_MAX_KEY_LENGTH, +) + ) + +yield "Cache key will cause errors if used with memcached: %r " "(longer than %s)" % ( + key, + MEMCACHE_MAX_KEY_LENGTH, +) + + +yield ("Unnecessary") + + +yield ( + "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " + "to the desired behavior" +) +yield ( + "# * Remove `managed = False` lines if you wish to allow " + "Django to create, modify, and delete the table" +) +yield ( + "# Feel free to rename the models, but don't rename db_table values or " + "field names." +) + +yield "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " "to the desired behavior" +yield "# * Remove `managed = False` lines if you wish to allow " "Django to create, modify, and delete the table" +yield "# Feel free to rename the models, but don't rename db_table values or " "field names." diff --git a/crates/ruff_python_formatter/src/expression/expr_await.rs b/crates/ruff_python_formatter/src/expression/expr_await.rs index 496b2ff763..b499e90173 100644 --- a/crates/ruff_python_formatter/src/expression/expr_await.rs +++ b/crates/ruff_python_formatter/src/expression/expr_await.rs @@ -20,7 +20,7 @@ impl FormatNodeRule for FormatExprAwait { [ text("await"), space(), - maybe_parenthesize_expression(value, item, Parenthesize::IfRequired) + maybe_parenthesize_expression(value, item, Parenthesize::IfBreaks) ] ) } diff --git a/crates/ruff_python_formatter/src/expression/expr_yield.rs b/crates/ruff_python_formatter/src/expression/expr_yield.rs index 337a6f95f0..077d0eaa89 100644 --- a/crates/ruff_python_formatter/src/expression/expr_yield.rs +++ b/crates/ruff_python_formatter/src/expression/expr_yield.rs @@ -90,7 +90,7 @@ impl Format> for AnyExpressionYield<'_> { [ text(keyword), space(), - maybe_parenthesize_expression(val, self, Parenthesize::IfRequired) + maybe_parenthesize_expression(val, self, Parenthesize::Optional) ] )?; } else { diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 2b55147044..75dce1be22 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -190,16 +190,13 @@ impl Format> for MaybeParenthesizeExpression<'_> { return expression.format().with_options(Parentheses::Always).fmt(f); } - let needs_parentheses = expression.needs_parentheses(*parent, f.context()); - let needs_parentheses = match parenthesize { - Parenthesize::IfRequired => match needs_parentheses { - OptionalParentheses::Always => OptionalParentheses::Always, - _ if f.context().node_level().is_parenthesized() => OptionalParentheses::Never, - needs_parentheses => needs_parentheses, - }, - Parenthesize::Optional - | Parenthesize::IfBreaks - | Parenthesize::IfBreaksOrIfRequired => needs_parentheses, + let needs_parentheses = match expression.needs_parentheses(*parent, f.context()) { + OptionalParentheses::Always => OptionalParentheses::Always, + // The reason to add parentheses is to avoid a syntax error when breaking an expression over multiple lines. + // Therefore, it is unnecessary to add an additional pair of parentheses if an outer expression + // is parenthesized. + _ if f.context().node_level().is_parenthesized() => OptionalParentheses::Never, + needs_parentheses => needs_parentheses, }; match needs_parentheses { diff --git a/crates/ruff_python_formatter/src/expression/parentheses.rs b/crates/ruff_python_formatter/src/expression/parentheses.rs index 1aaec853d1..748583ae0c 100644 --- a/crates/ruff_python_formatter/src/expression/parentheses.rs +++ b/crates/ruff_python_formatter/src/expression/parentheses.rs @@ -41,9 +41,8 @@ pub(crate) enum Parenthesize { /// Parenthesizes the expression only if it doesn't fit on a line. IfBreaks, - /// Only adds parentheses if absolutely necessary: - /// * The expression is not enclosed by another parenthesized expression and it expands over multiple lines - /// * The expression has leading or trailing comments. Adding parentheses is desired to prevent the comments from wandering. + /// Only adds parentheses if the expression has leading or trailing comments. + /// Adding parentheses is desired to prevent the comments from wandering. IfRequired, /// Parenthesizes the expression if the group doesn't fit on a line (e.g., even name expressions are parenthesized), or if diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap index 1698ba87e2..f8090247fc 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__yield.py.snap @@ -71,6 +71,38 @@ def foo(): test, # comment 4 1 ) + +yield ("Cache key will cause errors if used with memcached: %r " "(longer than %s)" % ( + key, + MEMCACHE_MAX_KEY_LENGTH, +) + ) + +yield "Cache key will cause errors if used with memcached: %r " "(longer than %s)" % ( + key, + MEMCACHE_MAX_KEY_LENGTH, +) + + +yield ("Unnecessary") + + +yield ( + "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " + "to the desired behavior" +) +yield ( + "# * Remove `managed = False` lines if you wish to allow " + "Django to create, modify, and delete the table" +) +yield ( + "# Feel free to rename the models, but don't rename db_table values or " + "field names." +) + +yield "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " "to the desired behavior" +yield "# * Remove `managed = False` lines if you wish to allow " "Django to create, modify, and delete the table" +yield "# Feel free to rename the models, but don't rename db_table values or " "field names." ``` ## Output @@ -110,7 +142,7 @@ def foo(): for e in l: # comment - yield e # Too many parentheses + yield (e) # Too many parentheses # comment for ridiculouslylongelementnameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee in l: @@ -135,6 +167,51 @@ def foo(): ) ) ) + + +yield ( + "Cache key will cause errors if used with memcached: %r " + "(longer than %s)" + % ( + key, + MEMCACHE_MAX_KEY_LENGTH, + ) +) + +yield "Cache key will cause errors if used with memcached: %r " "(longer than %s)" % ( + key, + MEMCACHE_MAX_KEY_LENGTH, +) + + +yield ("Unnecessary") + + +yield ( + "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " + "to the desired behavior" +) +yield ( + "# * Remove `managed = False` lines if you wish to allow " + "Django to create, modify, and delete the table" +) +yield ( + "# Feel free to rename the models, but don't rename db_table values or " + "field names." +) + +yield ( + "# * Make sure each ForeignKey and OneToOneField has `on_delete` set " + "to the desired behavior" +) +yield ( + "# * Remove `managed = False` lines if you wish to allow " + "Django to create, modify, and delete the table" +) +yield ( + "# Feel free to rename the models, but don't rename db_table values or " + "field names." +) ```