Skip to content

Commit

Permalink
Group function definition parameters with return type annotations (#6410
Browse files Browse the repository at this point in the history
)

## Summary

This PR removes the group around function definition parameters, instead
grouping the parameters with the type parameters and return type
annotation.

This increases Zulip's similarity score from 0.99385 to 0.99699, so it's
a meaningful improvement. However, there's at least one stability error
that I'm working on, and I'm really just looking for high-level feedback
at this point, because I'm not happy with the solution.

Closes #6352.

## Test Plan

Before:

- `zulip`: 0.99396
- `django`: 0.99784
- `warehouse`: 0.99578
- `build`: 0.75436
- `transformers`: 0.99407
- `cpython`: 0.75987
- `typeshed`: 0.74432

After:

- `zulip`: 0.99702
- `django`: 0.99784
- `warehouse`: 0.99585
- `build`: 0.75623
- `transformers`: 0.99470
- `cpython`: 0.75988
- `typeshed`: 0.74853
  • Loading branch information
charliermarsh committed Aug 9, 2023
1 parent eaada03 commit 3bf1c66
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -409,3 +409,29 @@ def double(a: int) -> ( # Hello
def double(a: int) -> ( # Hello
):
return 2*a

# Breaking over parameters and return types. (Black adds a trailing comma when the
# function arguments break here with a single argument; we do not.)
def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a:
...

def f(a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> a:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a:
...
18 changes: 13 additions & 5 deletions crates/ruff_python_formatter/src/other/parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{TextRange, TextSize};

use crate::comments::{
leading_comments, leading_node_comments, trailing_comments, CommentLinePosition, SourceComment,
dangling_open_parenthesis_comments, leading_comments, leading_node_comments, trailing_comments,
CommentLinePosition, SourceComment,
};
use crate::context::{NodeLevel, WithNodeLevel};
use crate::expression::parentheses::{empty_parenthesized, parenthesized};
use crate::expression::parentheses::empty_parenthesized;
use crate::prelude::*;
use crate::FormatNodeRule;

#[derive(Eq, PartialEq, Debug, Default)]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Default)]
pub enum ParametersParentheses {
/// By default, parameters will always preserve their surrounding parentheses.
#[default]
Expand Down Expand Up @@ -246,10 +247,17 @@ impl FormatNodeRule<Parameters> for FormatParameters {
// No parameters, format any dangling comments between `()`
write!(f, [empty_parenthesized("(", dangling, ")")])
} 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.
write!(
f,
[parenthesized("(", &group(&format_inner), ")")
.with_dangling_comments(parenthesis_dangling)]
[
text("("),
dangling_open_parenthesis_comments(parenthesis_dangling),
soft_block_indent(&group(&format_inner)),
text(")")
]
)
}
}
Expand Down
38 changes: 21 additions & 17 deletions crates/ruff_python_formatter/src/statement/stmt_function_def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,24 +58,28 @@ impl FormatNodeRule<StmtFunctionDef> for FormatStmtFunctionDef {
write!(f, [type_params.format()])?;
}

write!(f, [item.parameters.format()])?;

if let Some(return_annotation) = item.returns.as_ref() {
write!(f, [space(), text("->"), space()])?;
if return_annotation.is_tuple_expr() {
write!(
f,
[return_annotation.format().with_options(Parentheses::Never)]
)?;
} else {
write!(
f,
[optional_parentheses(
&return_annotation.format().with_options(Parentheses::Never),
)]
)?;
let format_inner = format_with(|f: &mut PyFormatter| {
write!(f, [item.parameters.format()])?;
if let Some(return_annotation) = item.returns.as_ref() {
write!(f, [space(), text("->"), space()])?;
if return_annotation.is_tuple_expr() {
write!(
f,
[return_annotation.format().with_options(Parentheses::Never)]
)?;
} else {
write!(
f,
[optional_parentheses(
&return_annotation.format().with_options(Parentheses::Never),
)]
)?;
}
}
}
Ok(())
});

write!(f, [group(&format_inner)])?;

write!(
f,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,20 @@ def foo() -> tuple[int, int, int,]:
```diff
--- Black
+++ Ruff
@@ -26,7 +26,9 @@
@@ -26,7 +26,11 @@
return 2 * a
-def double(a: int) -> int: # Hello
+def double(a: int) -> (
+def double(
+ a: int
+) -> (
+ int # Hello
+):
return 2 * a
@@ -54,7 +56,9 @@
@@ -54,7 +58,9 @@
a: int,
b: int,
c: int,
Expand Down Expand Up @@ -155,7 +157,9 @@ def double(a: int) -> int: # Hello
return 2 * a
def double(a: int) -> (
def double(
a: int
) -> (
int # Hello
):
return 2 * a
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,32 @@ def double(a: int) -> ( # Hello
def double(a: int) -> ( # Hello
):
return 2*a

# Breaking over parameters and return types. (Black adds a trailing comma when the
# function arguments break here with a single argument; we do not.)
def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a:
...

def f(a) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> a:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) -> a:
...
```

## Output
Expand Down Expand Up @@ -1023,9 +1049,61 @@ def double(a: int) -> int: # Hello
return 2 * a


def double(a: int) -> ( # Hello
def double(
a: int
) -> ( # Hello
):
return 2 * a


# Breaking over parameters and return types. (Black adds a trailing comma when the
# function arguments break here with a single argument; we do not.)
def f(
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...


def f(
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, a
) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...


def f(
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
) -> a:
...


def f(
a
) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...


def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]() -> (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
):
...


def f[
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
]() -> a:
...


def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
) -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...


def f[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa](
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
) -> a:
...
```


Expand Down

0 comments on commit 3bf1c66

Please sign in to comment.