Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve comprehension line break beheavior #5680

Merged
merged 1 commit into from
Jul 11, 2023
+123 −318
Merged

Improve comprehension line break beheavior #5680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,16 @@
# above g
g # g
]

[
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + [dddddddddddddddddd, eeeeeeeeeeeeeeeeeee]
for
ccccccccccccccccccccccccccccccccccccccc,
ddddddddddddddddddd, [eeeeeeeeeeeeeeeeeeeeee, fffffffffffffffffffffffff]
in
eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeffffffffffffffffffffffffggggggggggggggggggggghhhhhhhhhhhhhhothermoreeand_even_moreddddddddddddddddddddd
if
fffffffffffffffffffffffffffffffffffffffffff < gggggggggggggggggggggggggggggggggggggggggggggg < hhhhhhhhhhhhhhhhhhhhhhhhhh
if
gggggggggggggggggggggggggggggggggggggggggggg
]
48 changes: 26 additions & 22 deletions crates/ruff_python_formatter/src/expression/expr_compare.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,36 +33,40 @@ impl FormatNodeRule<ExprCompare> for FormatExprCompare {

let comments = f.context().comments().clone();

write!(f, [in_parentheses_only_group(&left.format())])?;
let inner = format_with(|f| {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change other than that this gets all wrapped in an in_parentheses_only_group

write!(f, [in_parentheses_only_group(&left.format())])?;

assert_eq!(comparators.len(), ops.len());

for (operator, comparator) in ops.iter().zip(comparators) {
let leading_comparator_comments = comments.leading_comments(comparator);
if leading_comparator_comments.is_empty() {
write!(f, [soft_line_break_or_space()])?;
} else {
// Format the expressions leading comments **before** the operator
write!(
f,
[
hard_line_break(),
leading_comments(leading_comparator_comments)
]
)?;
}

assert_eq!(comparators.len(), ops.len());

for (operator, comparator) in ops.iter().zip(comparators) {
let leading_comparator_comments = comments.leading_comments(comparator);
if leading_comparator_comments.is_empty() {
write!(f, [soft_line_break_or_space()])?;
} else {
// Format the expressions leading comments **before** the operator
write!(
f,
[
hard_line_break(),
leading_comments(leading_comparator_comments)
operator.format(),
space(),
in_parentheses_only_group(&comparator.format())
]
)?;
}

write!(
f,
[
operator.format(),
space(),
in_parentheses_only_group(&comparator.format())
]
)?;
}

Ok(())
Ok(())
});

in_parentheses_only_group(&inner).fmt(f)
}
}

Expand Down
35 changes: 27 additions & 8 deletions crates/ruff_python_formatter/src/other/comprehension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,25 @@ use crate::prelude::*;
use crate::AsFormat;
use crate::{FormatNodeRule, PyFormatter};
use ruff_formatter::{format_args, write, Buffer, FormatResult};
use rustpython_parser::ast::{Comprehension, Ranged};
use rustpython_parser::ast::{Comprehension, Expr, Ranged};

#[derive(Default)]
pub struct FormatComprehension;

impl FormatNodeRule<Comprehension> for FormatComprehension {
fn fmt_fields(&self, item: &Comprehension, f: &mut PyFormatter) -> FormatResult<()> {
struct Spacer<'a>(&'a Expr);

impl Format<PyFormatContext<'_>> for Spacer<'_> {
fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> {
if f.context().comments().has_leading_comments(self.0) {
soft_line_break_or_space().fmt(f)
} else {
space().fmt(f)
}
}
}

let Comprehension {
range: _,
target,
Expand All @@ -18,33 +30,40 @@ impl FormatNodeRule<Comprehension> for FormatComprehension {
is_async,
} = item;

let comments = f.context().comments().clone();

if *is_async {
write!(f, [text("async"), space()])?;
}

let comments = f.context().comments().clone();
let dangling_item_comments = comments.dangling_comments(item);

let (before_target_comments, before_in_comments) = dangling_item_comments.split_at(
dangling_item_comments
.partition_point(|comment| comment.slice().end() < target.range().start()),
);

let trailing_in_comments = comments.dangling_comments(iter);

let in_spacer = format_with(|f| {
if before_in_comments.is_empty() {
space().fmt(f)
} else {
soft_line_break_or_space().fmt(f)
}
});

write!(
f,
[
text("for"),
trailing_comments(before_target_comments),
group(&format_args!(
soft_line_break_or_space(),
Spacer(target),
target.format(),
soft_line_break_or_space(),
in_spacer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would the macros rules allow inlining the in_space as if expression?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because space and soft_line_break_or_space return different structs.

leading_comments(before_in_comments),
text("in"),
trailing_comments(trailing_in_comments),
soft_line_break_or_space(),
Spacer(iter),
iter.format(),
)),
]
Expand All @@ -64,7 +83,7 @@ impl FormatNodeRule<Comprehension> for FormatComprehension {
leading_comments(own_line_if_comments),
text("if"),
trailing_comments(end_of_line_if_comments),
soft_line_break_or_space(),
Spacer(if_case),
if_case.format(),
)));
}
Expand Down
21 changes: 5 additions & 16 deletions crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_37__python37.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def make_arange(n):
```diff
--- Black
+++ Ruff
@@ -2,29 +2,27 @@
@@ -2,14 +2,11 @@


def f():
Expand All @@ -59,17 +59,7 @@ def make_arange(n):


async def func():
if test:
out_batched = [
i
- async for i in aitertools._async_map(
- self.async_inc, arange(8), batch_size=3
- )
+ async for
+ i
+ in
+ aitertools._async_map(self.async_inc, arange(8), batch_size=3)
]
@@ -23,8 +20,8 @@


def awaited_generator_value(n):
Expand Down Expand Up @@ -100,10 +90,9 @@ async def func():
if test:
out_batched = [
i
async for
i
in
aitertools._async_map(self.async_inc, arange(8), batch_size=3)
async for i in aitertools._async_map(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that looks much nicer

self.async_inc, arange(8), batch_size=3
)
]


Expand Down
52 changes: 9 additions & 43 deletions ...ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments2.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,43 +224,18 @@ instruction()#comment with bad spacing
if (
self._proc is not None
# has the child process finished?
@@ -115,7 +123,12 @@
@@ -115,7 +123,9 @@
arg3=True,
)
lcomp = [
- element for element in collection if element is not None # yup # yup # right
+ element # yup
+ for
+ element
+ in
+ collection # yup
+ for element in collection # yup
+ if element is not None # right
]
lcomp2 = [
# hello
@@ -123,7 +136,9 @@
# yup
for element in collection
# right
- if element is not None
+ if
+ element
+ is not None
]
lcomp3 = [
# This one is actually too long to fit in a single line.
@@ -131,7 +146,9 @@
# yup
for element in collection.select_elements()
# right
- if element is not None
+ if
+ element
+ is not None
]
while True:
if False:
@@ -143,7 +160,10 @@
@@ -143,7 +153,10 @@
# let's return
return Node(
syms.simple_stmt,
Expand All @@ -272,14 +247,13 @@ instruction()#comment with bad spacing
)
@@ -158,7 +178,11 @@
@@ -158,7 +171,10 @@
class Test:
def _init_host(self, parsed) -> None:
- if parsed.hostname is None or not parsed.hostname.strip(): # type: ignore
+ if (
+ parsed.hostname
+ is None # type: ignore
+ parsed.hostname is None # type: ignore
+ or not parsed.hostname.strip()
+ ):
pass
Expand Down Expand Up @@ -416,10 +390,7 @@ short
)
lcomp = [
element # yup
for
element
in
collection # yup
for element in collection # yup
if element is not None # right
]
lcomp2 = [
Expand All @@ -428,19 +399,15 @@ short
# yup
for element in collection
# right
if
element
is not None
if element is not None
]
lcomp3 = [
# This one is actually too long to fit in a single line.
element.split("\n", 1)[0]
# yup
for element in collection.select_elements()
# right
if
element
is not None
if element is not None
]
while True:
if False:
Expand Down Expand Up @@ -471,8 +438,7 @@ CONFIG_FILES = (
class Test:
def _init_host(self, parsed) -> None:
if (
parsed.hostname
is None # type: ignore
parsed.hostname is None # type: ignore
or not parsed.hostname.strip()
):
pass
Expand Down
Loading