From 0c27a954266631d77c226c3769ec8a0cec148b09 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 7 Nov 2024 08:38:05 +0100 Subject: [PATCH] Prefer breaking return-type over parameters --- .../src/other/parameters.rs | 65 +++++--- ...funcdef_return_type_trailing_comma.py.snap | 56 +++---- ...cases__typed_params_trailing_comma.py.snap | 73 +++++++++ ...g_code_examples_dynamic_line_width.py.snap | 14 ++ ...ormat@statement__return_annotation.py.snap | 152 +++++++++++++++++- ...@statement__return_type_parameters.py.snap | 82 +++++++++- 6 files changed, 387 insertions(+), 55 deletions(-) create mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__typed_params_trailing_comma.py.snap diff --git a/crates/ruff_python_formatter/src/other/parameters.rs b/crates/ruff_python_formatter/src/other/parameters.rs index 7515ede0a0cc2..7fa28a3cb9b00 100644 --- a/crates/ruff_python_formatter/src/other/parameters.rs +++ b/crates/ruff_python_formatter/src/other/parameters.rs @@ -9,8 +9,9 @@ use crate::comments::{ trailing_comments, SourceComment, }; use crate::context::{NodeLevel, WithNodeLevel}; -use crate::expression::parentheses::empty_parenthesized; +use crate::expression::parentheses::{empty_parenthesized, parenthesized}; use crate::prelude::*; +use crate::preview::is_empty_parameters_no_unnecessary_parentheses_around_return_value_enabled; #[derive(Debug, Copy, Clone, Eq, PartialEq, Default)] pub enum ParametersParentheses { @@ -249,30 +250,46 @@ impl FormatNodeRule for FormatParameters { } else if num_parameters == 1 && posonlyargs.is_empty() && kwonlyargs.is_empty() { // If we have a single argument, avoid the inner group, to ensure that we insert a // trailing comma if the outer group breaks. - let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f); - write!( - f, - [ - token("("), - dangling_open_parenthesis_comments(parenthesis_dangling), - soft_block_indent(&format_inner), - token(")") - ] - ) + if is_empty_parameters_no_unnecessary_parentheses_around_return_value_enabled( + f.context(), + ) { + parenthesized("(", &format_inner, ")") + .with_dangling_comments(parenthesis_dangling) + .fmt(f) + } else { + let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f); + write!( + f, + [ + token("("), + dangling_open_parenthesis_comments(parenthesis_dangling), + soft_block_indent(&format_inner), + token(")") + ] + ) + } } else { - // Intentionally avoid `parenthesized`, which groups the entire formatted contents. - // We want parameters to be grouped alongside return types, one level up, so we - // format them "inline" here. - let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f); - write!( - f, - [ - token("("), - dangling_open_parenthesis_comments(parenthesis_dangling), - soft_block_indent(&group(&format_inner)), - token(")") - ] - ) + if is_empty_parameters_no_unnecessary_parentheses_around_return_value_enabled( + f.context(), + ) { + parenthesized("(", &group(&format_inner), ")") + .with_dangling_comments(parenthesis_dangling) + .fmt(f) + } else { + // Intentionally avoid `parenthesized`, which groups the entire formatted contents. + // We want parameters to be grouped alongside return types, one level up, so we + // format them "inline" here. + let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f); + write!( + f, + [ + token("("), + dangling_open_parenthesis_comments(parenthesis_dangling), + soft_block_indent(&group(&format_inner)), + token(")") + ] + ) + } } } } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__funcdef_return_type_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__funcdef_return_type_trailing_comma.py.snap index 7266806c08aa3..0e91c586bea09 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__funcdef_return_type_trailing_comma.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__funcdef_return_type_trailing_comma.py.snap @@ -1,6 +1,7 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/black/cases/funcdef_return_type_trailing_comma.py +snapshot_kind: text --- ## Input @@ -155,31 +156,38 @@ def SimplePyFn( ```diff --- Black +++ Ruff -@@ -36,7 +36,9 @@ +@@ -62,9 +62,9 @@ + # long function definition, return type is longer + # this should maybe split on rhs? +-def aaaaaaaaaaaaaaaaa( +- bbbbbbbbbbbbbbbbbb, +-) -> list[Ccccccccccccccccccccccccccccccccccccccccccccccccccc, Dddddd]: ... ++def aaaaaaaaaaaaaaaaa(bbbbbbbbbbbbbbbbbb) -> list[ ++ Ccccccccccccccccccccccccccccccccccccccccccccccccccc, Dddddd ++]: ... - # magic trailing comma in return type, params --def foo(a: A, b: B) -> list[ -+def foo( -+ a: A, b: B -+) -> list[ - p, - q, - ]: -@@ -93,7 +95,11 @@ + + # long return type, no param list +@@ -93,13 +93,13 @@ # unskippable type hint (??) -def foo(a) -> list[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]: # type: ignore -+def foo( -+ a, -+) -> list[ ++def foo(a) -> list[ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +]: # type: ignore pass -@@ -112,7 +118,13 @@ +-def foo( +- a, +-) -> list[ ++def foo(a) -> list[ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + ]: # abpedeifnore + pass +@@ -112,7 +112,13 @@ # don't lose any comments (no magic) @@ -194,7 +202,7 @@ def SimplePyFn( ... # 6 -@@ -120,12 +132,18 @@ +@@ -120,12 +126,18 @@ def foo( # 1 a, # 2 b, @@ -258,9 +266,7 @@ def a() -> tuple[ # magic trailing comma in return type, params -def foo( - a: A, b: B -) -> list[ +def foo(a: A, b: B) -> list[ p, q, ]: @@ -286,9 +292,9 @@ def aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa( # long function definition, return type is longer # this should maybe split on rhs? -def aaaaaaaaaaaaaaaaa( - bbbbbbbbbbbbbbbbbb, -) -> list[Ccccccccccccccccccccccccccccccccccccccccccccccccccc, Dddddd]: ... +def aaaaaaaaaaaaaaaaa(bbbbbbbbbbbbbbbbbb) -> list[ + Ccccccccccccccccccccccccccccccccccccccccccccccccccc, Dddddd +]: ... # long return type, no param list @@ -317,17 +323,13 @@ def thiiiiiiiiiiiiiiiiiis_iiiiiiiiiiiiiiiiiiiiiiiiiiiiiis_veeeeeeeeeeeeeeeeeeeee # unskippable type hint (??) -def foo( - a, -) -> list[ +def foo(a) -> list[ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ]: # type: ignore pass -def foo( - a, -) -> list[ +def foo(a) -> list[ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ]: # abpedeifnore pass diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__typed_params_trailing_comma.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__typed_params_trailing_comma.py.snap new file mode 100644 index 0000000000000..faadb0e7361b7 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@cases__typed_params_trailing_comma.py.snap @@ -0,0 +1,73 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/black/cases/typed_params_trailing_comma.py +snapshot_kind: text +--- +## Input + +```python +def long_function_name_goes_here( + x: Callable[List[int]] +) -> Union[List[int], float, str, bytes, Tuple[int]]: + pass + + +def long_function_name_goes_here( + x: Callable[[str, Any], int] +) -> Union[List[int], float, str, bytes, Tuple[int]]: + pass +``` + +## Black Differences + +```diff +--- Black ++++ Ruff +@@ -1,10 +1,10 @@ +-def long_function_name_goes_here( +- x: Callable[List[int]], +-) -> Union[List[int], float, str, bytes, Tuple[int]]: ++def long_function_name_goes_here(x: Callable[List[int]]) -> Union[ ++ List[int], float, str, bytes, Tuple[int] ++]: + pass + + +-def long_function_name_goes_here( +- x: Callable[[str, Any], int], +-) -> Union[List[int], float, str, bytes, Tuple[int]]: ++def long_function_name_goes_here(x: Callable[[str, Any], int]) -> Union[ ++ List[int], float, str, bytes, Tuple[int] ++]: + pass +``` + +## Ruff Output + +```python +def long_function_name_goes_here(x: Callable[List[int]]) -> Union[ + List[int], float, str, bytes, Tuple[int] +]: + pass + + +def long_function_name_goes_here(x: Callable[[str, Any], int]) -> Union[ + List[int], float, str, bytes, Tuple[int] +]: + pass +``` + +## Black Output + +```python +def long_function_name_goes_here( + x: Callable[List[int]], +) -> Union[List[int], float, str, bytes, Tuple[int]]: + pass + + +def long_function_name_goes_here( + x: Callable[[str, Any], int], +) -> Union[List[int], float, str, bytes, Tuple[int]]: + pass +``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@docstring_code_examples_dynamic_line_width.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@docstring_code_examples_dynamic_line_width.py.snap index fd884666c438c..389c7e90fa85c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@docstring_code_examples_dynamic_line_width.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@docstring_code_examples_dynamic_line_width.py.snap @@ -1,6 +1,7 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/docstring_code_examples_dynamic_line_width.py +snapshot_kind: text --- ## Input ```python @@ -2831,6 +2832,19 @@ def length_rst_in_section(): ```diff --- Stable +++ Preview +@@ -676,9 +676,9 @@ + ... def __init__(self, df: pl.DataFrame): + ... self._df = df + ... +- ... def by_first_letter_of_column_values( +- ... self, col: str +- ... ) -> list[pl.DataFrame]: ++ ... def by_first_letter_of_column_values(self, col: str) -> list[ ++ ... pl.DataFrame ++ ... ]: + ... return [ + ... self._df.filter(pl.col(col).str.starts_with(c)) + ... for c in sorted( @@ -700,9 +700,11 @@ Examples diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__return_annotation.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__return_annotation.py.snap index 86f834791af1f..1631c010e338b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__return_annotation.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__return_annotation.py.snap @@ -1,6 +1,7 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/return_annotation.py +snapshot_kind: text --- ## Input ```python @@ -525,7 +526,27 @@ def process_board_action( ```diff --- Stable +++ Preview -@@ -131,32 +131,24 @@ +@@ -70,17 +70,13 @@ + return 2 * a + + +-def double( +- a: int, +-) -> ( # Hello ++def double(a: int) -> ( # Hello + int + ): + return 2 * a + + +-def double( +- a: int, +-) -> ( # Hello ++def double(a: int) -> ( # Hello + ): + return 2 * a + +@@ -131,51 +127,37 @@ # Breaking return type annotations. Black adds parentheses if the parameters are # empty; otherwise, it leverages the expressions own parentheses if possible. @@ -569,8 +590,77 @@ def process_board_action( +]: ... - def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( -@@ -257,11 +249,8 @@ +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> Set[ ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> Set[ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + ]: ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> Set[ ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> Set[ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + ]: ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- *args, +-) -> Set[ ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(*args) -> Set[ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + ]: ... + +@@ -220,9 +202,9 @@ + ): ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> X + Y + foooooooooooooooooooooooooooooooooooo(): ... ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> ( ++ X + Y + foooooooooooooooooooooooooooooooooooo() ++): ... + + + def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> ( +@@ -230,9 +212,9 @@ + ): ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> X and Y and foooooooooooooooooooooooooooooooooooo(): ... ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> ( ++ X and Y and foooooooooooooooooooooooooooooooooooo() ++): ... + + + def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> ( +@@ -240,9 +222,9 @@ + ): ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> X | Y | foooooooooooooooooooooooooooooooooooo(): ... ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> ( ++ X | Y | foooooooooooooooooooooooooooooooooooo() ++): ... + + + def xxxxxxxxxxxxxxxxxxxxxxxxxxxx() -> ( +@@ -250,58 +232,43 @@ + ): ... + + +-def xxxxxxxxxxxxxxxxxxxxxxxxxxxx( +- x, +-) -> ( ++def xxxxxxxxxxxxxxxxxxxxxxxxxxxx(x) -> ( + X | Y | foooooooooooooooooooooooooooooooooooo() # comment ): ... @@ -584,4 +674,60 @@ def process_board_action( ): return 2 * a + + # Dangling comments on return annotations. +-def double( +- a: int, +-) -> ( ++def double(a: int) -> ( + int # Hello + ): + return 2 * a + + +-def double( +- a: int, +-) -> ( ++def double(a: int) -> ( + foo.bar # Hello + ): + return 2 * a + + +-def double( +- a: int, +-) -> ( ++def double(a: int) -> ( + [int] # Hello + ): + return 2 * a + + +-def double( +- a: int, +-) -> ( ++def double(a: int) -> ( + int | list[int] # Hello + ): + pass + + +-def double( +- a: int, +-) -> ( ++def double(a: int) -> ( + int + | list[ + int, int, int, int, int, int, int, int, int, int, int, int, int, int, int, int +@@ -310,7 +277,7 @@ + pass + + +-def process_board_action( +- payload: WildValue, action_type: Optional[str] +-) -> Optional[Tuple[str, str]]: ++def process_board_action(payload: WildValue, action_type: Optional[str]) -> Optional[ ++ Tuple[str, str] ++]: + pass ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__return_type_parameters.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__return_type_parameters.py.snap index 20d605b884317..3233399a2095c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__return_type_parameters.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__return_type_parameters.py.snap @@ -1,6 +1,7 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/return_type_parameters.py +snapshot_kind: text --- ## Input ```python @@ -418,7 +419,27 @@ def test_return_multiline_string_binary_expression_return_type_annotation( ```diff --- Stable +++ Preview -@@ -82,13 +82,13 @@ +@@ -58,17 +58,13 @@ + ######################################################################################### + + +-def test_return_multiline_string_type_annotation( +- a, +-) -> """str ++def test_return_multiline_string_type_annotation(a) -> """str + | list[str] + """: + pass + + +-def test_return_multiline_string_binary_expression_return_type_annotation( +- a, +-) -> ( ++def test_return_multiline_string_binary_expression_return_type_annotation(a) -> ( + """str + | list[str] + """ +@@ -82,13 +78,13 @@ ######################################################################################### @@ -434,4 +455,63 @@ def test_return_multiline_string_binary_expression_return_type_annotation( pass +@@ -110,40 +106,32 @@ + + + # Unlike with no-parameters, the return type gets never parenthesized. +-def parameters_overlong_subscript_return_type_with_single_element( +- a, +-) -> list[xxxxxxxxxxxxxxxxxxxxx]: ++def parameters_overlong_subscript_return_type_with_single_element(a) -> list[ ++ xxxxxxxxxxxxxxxxxxxxx ++]: + pass + + +-def parameters_subscript_return_type_multiple_elements( +- a, +-) -> list[ ++def parameters_subscript_return_type_multiple_elements(a) -> list[ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ]: + pass + + +-def parameters_subscript_return_type_multiple_overlong_elements( +- a, +-) -> list[ ++def parameters_subscript_return_type_multiple_overlong_elements(a) -> list[ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, + ]: + pass + + +-def parameters_subscriptreturn_type_with_overlong_value_( +- a, +-) -> liiiiiiiiiiiiiiiiiiiiist[ ++def parameters_subscriptreturn_type_with_overlong_value_(a) -> liiiiiiiiiiiiiiiiiiiiist[ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ]: + pass + + +-def parameters_overlong_subscript_return_type_with_overlong_single_element( +- a, +-) -> list[ ++def parameters_overlong_subscript_return_type_with_overlong_single_element(a) -> list[ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ]: + pass +@@ -185,9 +173,7 @@ + + + # Black only splits the return type. Ruff also breaks the parameters. This is probably a bug. +-def parameters_subscriptreturn_type_with_overlong_value_( +- a, +-) -> liiiiiiiiiiiiiiiiiiiiist[ ++def parameters_subscriptreturn_type_with_overlong_value_(a) -> liiiiiiiiiiiiiiiiiiiiist[ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx, + ]: ```