Skip to content

Commit

Permalink
Parenthesize multi-context managers
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Dec 21, 2023
1 parent d835b28 commit 7940671
Show file tree
Hide file tree
Showing 13 changed files with 928 additions and 649 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[
{
"target_version": "py38"
},
{
"target_version": "py39",
"preview": "enabled"
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[
{
"target_version": "py39",
"preview": "enabled"
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
if True:
with (
anyio.CancelScope(shield=True)
if get_running_loop()
else contextlib.nullcontext()
):
pass


# Black avoids parenthesizing the with because it can make all with items fit by just breaking
# around parentheses. We don't implement this optimisation because it makes it difficult to see where
# the different context managers start and end.
with cmd, xxxxxxxx.some_kind_of_method(
some_argument=[
"first",
"second",
"third",
]
) as cmd, another, and_more as x:
pass

# Avoid parenthesizing single item context managers when splitting after the parentheses (can_omit_optional_parentheses)
# is sufficient
with xxxxxxxx.some_kind_of_method(
some_argument=[
"first",
"second",
"third",
]
).another_method(): pass

if True:
with (
anyio.CancelScope(shield=True)
if get_running_loop()
else contextlib.nullcontext()
):
pass


# Black avoids parentheses here because it can make the entire with
# header fit without requiring parentheses to do so.
# We don't implement this optimisation because it very difficult to see where
# the different context managers start or end.
with cmd, xxxxxxxx.some_kind_of_method(
some_argument=[
"first",
"second",
"third",
]
) as cmd, another, and_more as x:
pass

# Avoid parenthesizing single item context managers when splitting after the parentheses
# is sufficient
with xxxxxxxx.some_kind_of_method(
some_argument=[
"first",
"second",
"third",
]
).another_method(): pass

# Parenthesize the with items if it makes them fit. Breaking the binary expression isn't
# necessary because the entire items fit just into the 88 character limit.
with aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c:
pass


# Black parenthesizes this binary expression but also preserves the parentheses of the first with-item.
# It does so because it prefers splitting already parenthesized context managers, even if it leads to more parentheses
# like in this case.
with (
(
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
) as b,
c as d,
):
pass

if True:
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext():
pass

with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c):
pass


21 changes: 15 additions & 6 deletions crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ impl<'ast> IntoFormat<PyFormatContext<'ast>> for Expr {
///
/// This mimics Black's [`_maybe_split_omitting_optional_parens`](https://github.com/psf/black/blob/d1248ca9beaf0ba526d265f4108836d89cf551b7/src/black/linegen.py#L746-L820)
#[allow(clippy::if_same_then_else)]
fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool {
pub(crate) fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool {
let mut visitor = CanOmitOptionalParenthesesVisitor::new(context);
visitor.visit_subexpression(expr);

Expand All @@ -538,8 +538,8 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool
false
} else if visitor.max_precedence_count > 1 {
false
} else if visitor.max_precedence == OperatorPrecedence::None && expr.is_lambda_expr() {
// Micha: This seems to exclusively apply for lambda expressions where the body ends in a subscript.
} else if visitor.max_precedence == OperatorPrecedence::None {
// Micha: This seems to apply for lambda expressions where the body ends in a subscript.
// Subscripts are excluded by default because breaking them looks odd, but it seems to be fine for lambda expression.
//
// ```python
Expand All @@ -566,10 +566,19 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool
// ]
// )
// ```
//
// Another case are method chains:
// ```python
// xxxxxxxx.some_kind_of_method(
// some_argument=[
// "first",
// "second",
// "third",
// ]
// ).another_method(a)
// ```
true
} else if visitor.max_precedence == OperatorPrecedence::Attribute
&& (expr.is_lambda_expr() || expr.is_named_expr_expr())
{
} else if visitor.max_precedence == OperatorPrecedence::Attribute {
// A single method call inside a named expression (`:=`) or as the body of a lambda function:
// ```python
// kwargs["open_with"] = lambda path, _: fsspec.open(
Expand Down
55 changes: 39 additions & 16 deletions crates/ruff_python_formatter/src/other/with_item.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use ruff_formatter::write;

use ruff_python_ast::WithItem;

use crate::comments::SourceComment;
Expand All @@ -8,6 +7,7 @@ use crate::expression::parentheses::{
is_expression_parenthesized, parenthesized, Parentheses, Parenthesize,
};
use crate::prelude::*;
use crate::preview::is_wrap_multiple_context_managers_in_parens_enabled;

#[derive(Default)]
pub struct FormatWithItem;
Expand All @@ -23,26 +23,49 @@ impl FormatNodeRule<WithItem> for FormatWithItem {
let comments = f.context().comments().clone();
let trailing_as_comments = comments.dangling(item);

// Prefer keeping parentheses for already parenthesized expressions over
// parenthesizing other nodes.
let parenthesize = if is_expression_parenthesized(
let is_parenthesized = is_expression_parenthesized(
context_expr.into(),
f.context().comments().ranges(),
f.context().source(),
) {
Parenthesize::IfBreaks
);

// Remove the parentheses of the `with_items` if the with statement adds parentheses
if f.context().node_level().is_parenthesized()
&& is_wrap_multiple_context_managers_in_parens_enabled(f.context())
{
if is_parenthesized {
// ...except if the with item is parenthesized, then use this with item as a preferred breaking point
// or when it has comments, then parenthesize it to prevent comments from moving.
maybe_parenthesize_expression(
context_expr,
item,
Parenthesize::IfBreaksOrIfRequired,
)
.fmt(f)?;
} else {
context_expr
.format()
.with_options(Parentheses::Never)
.fmt(f)?;
}
} else {
Parenthesize::IfRequired
};
// Prefer keeping parentheses for already parenthesized expressions over
// parenthesizing other nodes.
let parenthesize = if is_parenthesized {
Parenthesize::IfBreaks
} else {
Parenthesize::IfRequired
};

write!(
f,
[maybe_parenthesize_expression(
context_expr,
item,
parenthesize
)]
)?;
write!(
f,
[maybe_parenthesize_expression(
context_expr,
item,
parenthesize
)]
)?;
}

if let Some(optional_vars) = optional_vars {
write!(f, [space(), token("as"), space()])?;
Expand Down
9 changes: 9 additions & 0 deletions crates/ruff_python_formatter/src/preview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,12 @@ pub(crate) const fn is_no_blank_line_before_class_docstring_enabled(
) -> bool {
context.is_preview()
}

/// Returns `true` if the [`wrap_multiple_context_managers_in_parens`](https://github.com/astral-sh/ruff/issues/8889) preview style is enabled.
///
/// Unlike Black, we re-use the same preview style feature flag for [`improved_async_statements_handling`](https://github.com/astral-sh/ruff/issues/8890)
pub(crate) const fn is_wrap_multiple_context_managers_in_parens_enabled(
context: &PyFormatContext,
) -> bool {
context.is_preview()
}
Loading

0 comments on commit 7940671

Please sign in to comment.