From b11204975827a722d4e3a6da8f0d52dc10ffb6d8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 9 Mar 2024 00:56:02 +0100 Subject: [PATCH] Formatter: Improve single-with item formatting for Python 3.8 or older (#10276) ## Summary This PR changes how we format `with` statements with a single with item for Python 3.8 or older. This change is not compatible with Black. This is how we format a single-item with statement today ```python def run(data_path, model_uri): with pyspark.sql.SparkSession.builder.config( key="spark.python.worker.reuse", value=True ).config(key="spark.ui.enabled", value=False).master( "local-cluster[2, 1, 1024]" ).getOrCreate(): # ignore spark log output spark.sparkContext.setLogLevel("OFF") print(score_model(spark, data_path, model_uri)) ``` This is different than how we would format the same expression if it is inside any other clause header (`while`, `if`, ...): ```python def run(data_path, model_uri): while ( pyspark.sql.SparkSession.builder.config( key="spark.python.worker.reuse", value=True ) .config(key="spark.ui.enabled", value=False) .master("local-cluster[2, 1, 1024]") .getOrCreate() ): # ignore spark log output spark.sparkContext.setLogLevel("OFF") print(score_model(spark, data_path, model_uri)) ``` Which seems inconsistent to me. This PR changes the formatting of the single-item with Python 3.8 or older to match that of other clause headers. ```python def run(data_path, model_uri): with ( pyspark.sql.SparkSession.builder.config( key="spark.python.worker.reuse", value=True ) .config(key="spark.ui.enabled", value=False) .master("local-cluster[2, 1, 1024]") .getOrCreate() ): # ignore spark log output spark.sparkContext.setLogLevel("OFF") print(score_model(spark, data_path, model_uri)) ``` According to our versioning policy, this style change is gated behind a preview flag. ## Test Plan See added tests. Added --- .../test/fixtures/ruff/statement/with.py | 10 ++ .../src/other/with_item.rs | 9 +- crates/ruff_python_formatter/src/preview.rs | 4 + .../src/statement/stmt_with.rs | 4 +- .../snapshots/format@statement__with.py.snap | 144 ++++++++++++++++++ 5 files changed, 167 insertions(+), 4 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py index 5b92ed3f7f766..f987b0856ed9f 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py @@ -318,3 +318,13 @@ ) ): pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: + pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b: + pass + +if True: + with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as b: + pass diff --git a/crates/ruff_python_formatter/src/other/with_item.rs b/crates/ruff_python_formatter/src/other/with_item.rs index fdd4cb54c855d..fc27bdf87338e 100644 --- a/crates/ruff_python_formatter/src/other/with_item.rs +++ b/crates/ruff_python_formatter/src/other/with_item.rs @@ -7,6 +7,7 @@ use crate::expression::parentheses::{ is_expression_parenthesized, parenthesized, Parentheses, Parenthesize, }; use crate::prelude::*; +use crate::preview::is_with_single_item_pre_39_enabled; #[derive(Copy, Clone, Debug, Eq, PartialEq, Default)] pub enum WithItemLayout { @@ -49,7 +50,7 @@ pub enum WithItemLayout { /// with a, b: /// ... /// ``` - Python38OrOlder, + Python38OrOlder { single: bool }, /// A with item where the `with` formatting adds parentheses around all context managers if necessary. /// @@ -135,8 +136,10 @@ impl FormatNodeRule for FormatWithItem { )?; } - WithItemLayout::Python38OrOlder => { - let parenthesize = if is_parenthesized { + WithItemLayout::Python38OrOlder { single } => { + let parenthesize = if (single && is_with_single_item_pre_39_enabled(f.context())) + || is_parenthesized + { Parenthesize::IfBreaks } else { Parenthesize::IfRequired diff --git a/crates/ruff_python_formatter/src/preview.rs b/crates/ruff_python_formatter/src/preview.rs index 067a0af7e9e53..a403e4a8011d4 100644 --- a/crates/ruff_python_formatter/src/preview.rs +++ b/crates/ruff_python_formatter/src/preview.rs @@ -18,3 +18,7 @@ pub(crate) const fn is_hug_parens_with_braces_and_square_brackets_enabled( pub(crate) fn is_f_string_formatting_enabled(context: &PyFormatContext) -> bool { context.is_preview() } + +pub(crate) fn is_with_single_item_pre_39_enabled(context: &PyFormatContext) -> bool { + context.is_preview() +} diff --git a/crates/ruff_python_formatter/src/statement/stmt_with.rs b/crates/ruff_python_formatter/src/statement/stmt_with.rs index 00d2fe31c4602..d0f838eab3176 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_with.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_with.rs @@ -104,7 +104,9 @@ impl FormatNodeRule for FormatStmtWith { WithItemsLayout::Python38OrOlder => f .join_with(format_args![token(","), space()]) .entries(with_stmt.items.iter().map(|item| { - item.format().with_options(WithItemLayout::Python38OrOlder) + item.format().with_options(WithItemLayout::Python38OrOlder { + single: with_stmt.items.len() == 1, + }) })) .finish(), diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap index 4b3dc56389a95..53f0860195b1e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap @@ -324,6 +324,16 @@ with ( ) ): pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: + pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b: + pass + +if True: + with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as b: + pass ``` ## Outputs @@ -688,6 +698,121 @@ with open( "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization ): pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: + pass + +with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b: + pass + +if True: + with anyio.CancelScope( + shield=True + ) if get_running_loop() else contextlib.nullcontext() as b: + pass +``` + + +#### Preview changes +```diff +--- Stable ++++ Preview +@@ -49,7 +49,9 @@ + with a: # should remove brackets + pass + +-with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c: ++with ( ++ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ++) as c: + pass + + +@@ -214,7 +216,9 @@ + pass + + # Breaking of with items. +-with test as ( # bar # foo ++with ( ++ test # bar ++) as ( # foo + # test + foo + ): +@@ -226,7 +230,9 @@ + ): + pass + +-with test as ( # bar # foo # baz ++with ( ++ test # bar ++) as ( # foo # baz + # test + foo + ): +@@ -279,7 +285,9 @@ + ) as b: + pass + +-with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b: ++with ( ++ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ++) as b: + pass + + with ( +@@ -322,15 +330,19 @@ + pass + + if True: +- with anyio.CancelScope( +- shield=True +- ) if get_running_loop() else contextlib.nullcontext(): ++ with ( ++ anyio.CancelScope(shield=True) ++ if get_running_loop() ++ else contextlib.nullcontext() ++ ): + pass + + if True: +- with anyio.CancelScope( +- shield=True +- ) if get_running_loop() else contextlib.nullcontext() as c: ++ with ( ++ anyio.CancelScope(shield=True) ++ if get_running_loop() ++ else contextlib.nullcontext() ++ ) as c: + pass + + with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document( +@@ -344,14 +356,20 @@ + ): + pass + +-with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb: ++with ( ++ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ++): + pass + +-with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b: ++with ( ++ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ++) as b: + pass + + if True: +- with anyio.CancelScope( +- shield=True +- ) if get_running_loop() else contextlib.nullcontext() as b: ++ with ( ++ anyio.CancelScope(shield=True) ++ if get_running_loop() ++ else contextlib.nullcontext() ++ ) as b: + pass ``` @@ -1093,4 +1218,23 @@ with open( "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization ): pass + +with ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb +): + pass + +with ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as b +): + pass + +if True: + with ( + anyio.CancelScope(shield=True) + if get_running_loop() + else contextlib.nullcontext() as b + ): + pass ```