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.

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

Fix formatting of chained boolean operations #6394

Merged
merged 13 commits into from
Aug 7, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,14 @@
and [dddddddddddddd, eeeeeeeeee, fffffffffffffff]
):
pass

# Regression test for https://github.com/astral-sh/ruff/issues/6068
if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or numpy and isinstance(ccccccccccc, dddddd)
):
pass

if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and numpy or isinstance(ccccccccccc, dddddd)
):
pass
55 changes: 42 additions & 13 deletions crates/ruff_python_formatter/src/expression/expr_bool_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,24 @@ use crate::expression::parentheses::{
use crate::prelude::*;
use ruff_formatter::{write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{BoolOp, ExprBoolOp};
use ruff_python_ast::{BoolOp, Expr, ExprBoolOp};

#[derive(Default)]
pub struct FormatExprBoolOp {
parentheses: Option<Parentheses>,
chained: bool,
}

pub struct BoolOpLayout {
pub(crate) parentheses: Option<Parentheses>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused. Will remove in follow-up pull request.

pub(crate) chained: bool,
}

impl FormatRuleWithOptions<ExprBoolOp, PyFormatContext<'_>> for FormatExprBoolOp {
type Options = Option<Parentheses>;
type Options = BoolOpLayout;
fn with_options(mut self, options: Self::Options) -> Self {
self.parentheses = options;
self.parentheses = options.parentheses;
self.chained = options.chained;
self
}
}
Expand All @@ -37,7 +44,19 @@ impl FormatNodeRule<ExprBoolOp> for FormatExprBoolOp {
return Ok(());
};

write!(f, [in_parentheses_only_group(&first.format())])?;
if let Expr::BoolOp(value) = first {
// Mark chained boolean operations e.g. `x and y or z`
// and avoid creating a new group
write!(
f,
[value.format().with_options(BoolOpLayout {
parentheses: None,
chained: true,
})]
)?;
} else {
write!(f, [in_parentheses_only_group(&first.format())])?;
}

for value in values {
let leading_value_comments = comments.leading_comments(value);
Expand All @@ -51,20 +70,30 @@ impl FormatNodeRule<ExprBoolOp> for FormatExprBoolOp {
)?;
}

write!(
f,
[
op.format(),
space(),
in_parentheses_only_group(&value.format())
]
)?;
write!(f, [op.format(), space(),])?;

if let Expr::BoolOp(value) = value {
write!(
f,
[value.format().with_options(BoolOpLayout {
parentheses: None,
chained: true,
})]
)?;
} else {
write!(f, [value.format()])?;
}
}

Ok(())
});

in_parentheses_only_group(&inner).fmt(f)
if self.chained {
// Chained boolean operations should not be given a new group
inner.fmt(f)
} else {
in_parentheses_only_group(&inner).fmt(f)
}
}
}

Expand Down
10 changes: 9 additions & 1 deletion crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use crate::expression::parentheses::{
};
use crate::prelude::*;

use self::expr_bool_op::BoolOpLayout;

pub(crate) mod expr_attribute;
pub(crate) mod expr_await;
pub(crate) mod expr_bin_op;
Expand Down Expand Up @@ -67,7 +69,13 @@ impl FormatRule<Expr, PyFormatContext<'_>> for FormatExpr {
let parentheses = self.parentheses;

let format_expr = format_with(|f| match expression {
Expr::BoolOp(expr) => expr.format().with_options(Some(parentheses)).fmt(f),
Expr::BoolOp(expr) => expr
.format()
.with_options(BoolOpLayout {
parentheses: Some(parentheses),
chained: false,
})
.fmt(f),
Expr::NamedExpr(expr) => expr.format().fmt(f),
Expr::BinOp(expr) => expr.format().fmt(f),
Expr::UnaryOp(expr) => expr.format().fmt(f),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,24 @@ last_call()
```diff
--- Black
+++ Ruff
@@ -12,13 +12,13 @@
True or False or None
True and False
True and False and None
-(Name1 and Name2) or Name3
Name1 and Name2 or Name3
-Name1 or (Name2 and Name3)
+Name1 and Name2 or Name3
+Name1 or Name2 and Name3
Name1 or Name2 and Name3
-(Name1 and Name2) or (Name3 and Name4)
Name1 and Name2 or Name3 and Name4
-Name1 or (Name2 and Name3) or Name4
+Name1 and Name2 or Name3 and Name4
+Name1 or Name2 and Name3 or Name4
Name1 or Name2 and Name3 or Name4
v1 << 2
1 >> v2
@@ -31,7 +31,7 @@
-1
~int and not v1 ^ 123 + v2 | True
Expand Down Expand Up @@ -351,13 +369,13 @@ True or False
True or False or None
True and False
True and False and None
(Name1 and Name2) or Name3
Name1 and Name2 or Name3
Name1 or (Name2 and Name3)
Name1 and Name2 or Name3
Name1 or Name2 and Name3
Name1 or Name2 and Name3
(Name1 and Name2) or (Name3 and Name4)
Name1 and Name2 or Name3 and Name4
Name1 or (Name2 and Name3) or Name4
Name1 and Name2 or Name3 and Name4
Name1 or Name2 and Name3 or Name4
zanieb marked this conversation as resolved.
Show resolved Hide resolved
Name1 or Name2 and Name3 or Name4
v1 << 2
1 >> v2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ def test():
", %s unmodified" % unmodified_count if collected["unmodified"] else ""
),
"post_processed": (
collected["post_processed"] and ", %s post-processed" % post_processed_count
collected["post_processed"]
and ", %s post-processed" % post_processed_count
or ""
),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ if (
and [dddddddddddddd, eeeeeeeeee, fffffffffffffff]
):
pass

# Regression test for https://github.com/astral-sh/ruff/issues/6068
if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) or numpy and isinstance(ccccccccccc, dddddd)
):
pass

if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb) and numpy or isinstance(ccccccccccc, dddddd)
):
pass
```

## Output
Expand Down Expand Up @@ -136,6 +147,21 @@ if (
and [dddddddddddddd, eeeeeeeeee, fffffffffffffff]
):
pass

# Regression test for https://github.com/astral-sh/ruff/issues/6068
if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb)
or numpy
and isinstance(ccccccccccc, dddddd)
):
pass

if not (
isinstance(aaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbb)
and numpy
or isinstance(ccccccccccc, dddddd)
):
pass
```
Copy link
Member Author

Choose a reason for hiding this comment

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

Yay!



Expand Down