mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-07-23 13:05:06 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Avoid breaking call chains unnecessarily (#6488)

Browse source 
## Summary

This PR attempts to fix the formatting of the following expression:

```python
max_message_id = (
    Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id
)
```

Specifically, Black preserves _that_ formatting, while we do:

```python
max_message_id = (
    Message.objects.filter(recipient=recipient)
    .order_by("id")
    .reverse()[0]
    .id
)
```

The fix here is to add a group around the entire call chain.

## Test Plan

Before:

- `zulip`: 0.99702
- `django`: 0.99784
- `warehouse`: 0.99585
- `build`: 0.75623
- `transformers`: 0.99470
- `cpython`: 0.75989
- `typeshed`: 0.74853

After:

- `zulip`: 0.99703
- `django`: 0.99791
- `warehouse`: 0.99586
- `build`: 0.75623
- `transformers`: 0.99470
- `cpython`: 0.75989
- `typeshed`: 0.74853
This commit is contained in:
Charlie Marsh 2023-08-11 09:33:15 -04:00 • committed by GitHub
parent b05574babd
commit f2939c678b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 122 additions and 91 deletions
Download patch file Download diff file Expand all files Collapse all files

6
crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/call_chains.py vendored
View file

@ -154,4 +154,10 @@ zero(
five, five,
) )
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id
)
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id()
)

193
crates/ruff_python_formatter/src/expression/expr_attribute.rs
View file

@ -35,105 +35,116 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
let call_chain_layout = self.call_chain_layout.apply_in_node(item, f); let call_chain_layout = self.call_chain_layout.apply_in_node(item, f);
let needs_parentheses = matches!( let format_inner = format_with(|f: &mut PyFormatter| {
value.as_ref(), let needs_parentheses = matches!(
Expr::Constant(ExprConstant { value.as_ref(),
value: Constant::Int(_) | Constant::Float(_), Expr::Constant(ExprConstant {
.. value: Constant::Int(_) | Constant::Float(_),
}) ..
); })
);
let comments = f.context().comments().clone(); let comments = f.context().comments().clone();
let dangling_comments = comments.dangling_comments(item); let dangling_comments = comments.dangling_comments(item);
let leading_attribute_comments_start = let leading_attribute_comments_start = dangling_comments
dangling_comments.partition_point(|comment| comment.line_position().is_end_of_line()); .partition_point(|comment| comment.line_position().is_end_of_line());
let (trailing_dot_comments, leading_attribute_comments) = let (trailing_dot_comments, leading_attribute_comments) =
dangling_comments.split_at(leading_attribute_comments_start); dangling_comments.split_at(leading_attribute_comments_start);
if needs_parentheses { if needs_parentheses {
value.format().with_options(Parentheses::Always).fmt(f)?; value.format().with_options(Parentheses::Always).fmt(f)?;
} else if call_chain_layout == CallChainLayout::Fluent { } else if call_chain_layout == CallChainLayout::Fluent {
match value.as_ref() { match value.as_ref() {
Expr::Attribute(expr) => { Expr::Attribute(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?; expr.format().with_options(call_chain_layout).fmt(f)?;
} }
Expr::Call(expr) => { Expr::Call(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?; expr.format().with_options(call_chain_layout).fmt(f)?;
if call_chain_layout == CallChainLayout::Fluent { if call_chain_layout == CallChainLayout::Fluent {
// Format the dot on its own line // Format the dot on its own line
soft_line_break().fmt(f)?; soft_line_break().fmt(f)?;
} }
} }
Expr::Subscript(expr) => { Expr::Subscript(expr) => {
expr.format().with_options(call_chain_layout).fmt(f)?; expr.format().with_options(call_chain_layout).fmt(f)?;
if call_chain_layout == CallChainLayout::Fluent { if call_chain_layout == CallChainLayout::Fluent {
// Format the dot on its own line // Format the dot on its own line
soft_line_break().fmt(f)?; soft_line_break().fmt(f)?;
} }
} }
_ => { _ => {
// This matches [`CallChainLayout::from_expression`] // This matches [`CallChainLayout::from_expression`]
if is_expression_parenthesized(value.as_ref().into(), f.context().source()) { if is_expression_parenthesized(value.as_ref().into(), f.context().source())
value.format().with_options(Parentheses::Always).fmt(f)?; {
// Format the dot on its own line value.format().with_options(Parentheses::Always).fmt(f)?;
soft_line_break().fmt(f)?; // Format the dot on its own line
} else { soft_line_break().fmt(f)?;
value.format().fmt(f)?; } else {
value.format().fmt(f)?;
}
} }
} }
} else {
value.format().fmt(f)?;
} }
} else {
value.format().fmt(f)?;
}
if comments.has_trailing_own_line_comments(value.as_ref()) { if comments.has_trailing_own_line_comments(value.as_ref()) {
hard_line_break().fmt(f)?; hard_line_break().fmt(f)?;
} }
if call_chain_layout == CallChainLayout::Fluent { if call_chain_layout == CallChainLayout::Fluent {
// Fluent style has line breaks before the dot // Fluent style has line breaks before the dot
// ```python // ```python
// blogs3 = ( // blogs3 = (
// Blog.objects.filter( // Blog.objects.filter(
// entry__headline__contains="Lennon", // entry__headline__contains="Lennon",
// ) // )
// .filter( // .filter(
// entry__pub_date__year=2008, // entry__pub_date__year=2008,
// ) // )
// .filter( // .filter(
// entry__pub_date__year=2008, // entry__pub_date__year=2008,
// ) // )
// ) // )
// ``` // ```
write!( write!(
f, f,
[ [
(!leading_attribute_comments.is_empty()).then_some(hard_line_break()), (!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments), leading_comments(leading_attribute_comments),
text("."), text("."),
trailing_comments(trailing_dot_comments), trailing_comments(trailing_dot_comments),
attr.format() attr.format()
] ]
) )
} else {
// Regular style
// ```python
// blogs2 = Blog.objects.filter(
// entry__headline__contains="Lennon",
// ).filter(
// entry__pub_date__year=2008,
// )
// ```
write!(
f,
[
text("."),
trailing_comments(trailing_dot_comments),
(!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments),
attr.format()
]
)
}
});
let is_call_chain_root = self.call_chain_layout == CallChainLayout::Default
&& call_chain_layout == CallChainLayout::Fluent;
if is_call_chain_root {
write!(f, [group(&format_inner)])
} else { } else {
// Regular style write!(f, [format_inner])
// ```python
// blogs2 = Blog.objects.filter(
// entry__headline__contains="Lennon",
// ).filter(
// entry__pub_date__year=2008,
// )
// ```
write!(
f,
[
text("."),
trailing_comments(trailing_dot_comments),
(!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments),
attr.format()
]
)
} }
} }

14
crates/ruff_python_formatter/tests/snapshots/format@parentheses__call_chains.py.snap
View file

@ -160,7 +160,13 @@ zero(
five, five,
) )
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id
)
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id()
)
``` ```
## Output ## Output
@ -334,6 +340,14 @@ zero(
).four( ).four(
five, five,
) )
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id
)
max_message_id = (
Message.objects.filter(recipient=recipient).order_by("id").reverse()[0].id()
)
``` ```