Skip to content

Commit

Permalink
Refactor with statement formatting to have explicit layouts (#10296)
Browse files Browse the repository at this point in the history
## Summary

This PR refactors the with item formatting to use more explicit layouts
to make it easier to understand the different formatting cases.

The benefit of the explicit layout is that it makes it easier to reasons
about layout transition between format runs. For example, today it's
possible that `SingleWithTarget` or `ParenthesizeIfExpands` add
parentheses around the with items for `with aaaaaaaaaa + bbbbbbbbbbbb:
pass`, resulting in `with (aaaaaaaaaa + bbbbbbbbbbbb): pass`. The
problem with this is that the next formatting pass uses the
`SingleParenthesizedContextExpression` layout that uses
`maybe_parenthesize_expression` which is different from
`parenthesize_if_expands(&expr)` or `optional_parentheses(&expr)`.

## Test Plan

`cargo test`

I ran the ecosystem checks locally and there are no changes.
  • Loading branch information
MichaReiser committed Mar 8, 2024
1 parent 1d97f27 commit a56d42f
Show file tree
Hide file tree
Showing 5 changed files with 303 additions and 164 deletions.
1 change: 0 additions & 1 deletion crates/ruff_python_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> {
if let Some(last_end) = self.entries.position() {
let magic_trailing_comma = has_magic_trailing_comma(
TextRange::new(last_end, self.sequence_end),
self.fmt.options(),
self.fmt.context(),
);

Expand Down
6 changes: 1 addition & 5 deletions crates/ruff_python_formatter/src/other/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,7 @@ fn is_arguments_huggable(arguments: &Arguments, context: &PyFormatContext) -> bo

// If the expression has a trailing comma, then we can't hug it.
if options.magic_trailing_comma().is_respect()
&& commas::has_magic_trailing_comma(
TextRange::new(arg.end(), arguments.end()),
options,
context,
)
&& commas::has_magic_trailing_comma(TextRange::new(arg.end(), arguments.end()), context)
{
return false;
}
Expand Down
11 changes: 4 additions & 7 deletions crates/ruff_python_formatter/src/other/commas.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
use ruff_formatter::FormatContext;
use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::TextRange;

use crate::prelude::*;
use crate::{MagicTrailingComma, PyFormatOptions};
use crate::MagicTrailingComma;

/// Returns `true` if the range ends with a magic trailing comma (and the magic trailing comma
/// should be respected).
pub(crate) fn has_magic_trailing_comma(
range: TextRange,
options: &PyFormatOptions,
context: &PyFormatContext,
) -> bool {
match options.magic_trailing_comma() {
pub(crate) fn has_magic_trailing_comma(range: TextRange, context: &PyFormatContext) -> bool {
match context.options().magic_trailing_comma() {
MagicTrailingComma::Respect => {
let first_token = SimpleTokenizer::new(context.source(), range)
.skip_trivia()
Expand Down
138 changes: 104 additions & 34 deletions crates/ruff_python_formatter/src/other/with_item.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use ruff_formatter::write;
use ruff_formatter::{write, FormatRuleWithOptions};
use ruff_python_ast::WithItem;

use crate::comments::SourceComment;
Expand All @@ -8,8 +8,66 @@ use crate::expression::parentheses::{
};
use crate::prelude::*;

#[derive(Copy, Clone, Debug, Eq, PartialEq, Default)]
pub enum WithItemLayout {
/// A with item that is the `with`s only context manager and its context expression is parenthesized.
///
/// ```python
/// with (
/// a + b
/// ) as b:
/// ...
/// ```
///
/// This layout is used independent of the target version.
SingleParenthesizedContextManager,

/// This layout is used when the target python version doesn't support parenthesized context managers and
/// it's either a single, unparenthesized with item or multiple items.
///
/// ```python
/// with a + b:
/// ...
///
/// with a, b:
/// ...
/// ```
Python38OrOlder,

/// A with item where the `with` formatting adds parentheses around all context managers if necessary.
///
/// ```python
/// with (
/// a,
/// b,
/// ): pass
/// ```
///
/// This layout is generally used when the target version is Python 3.9 or newer, but it is used
/// for Python 3.8 if the with item has a leading or trailing comment.
///
/// ```python
/// with (
/// # leading
/// a
// ): ...
/// ```
#[default]
ParenthesizedContextManagers,
}

#[derive(Default)]
pub struct FormatWithItem;
pub struct FormatWithItem {
layout: WithItemLayout,
}

impl FormatRuleWithOptions<WithItem, PyFormatContext<'_>> for FormatWithItem {
type Options = WithItemLayout;

fn with_options(self, options: Self::Options) -> Self {
Self { layout: options }
}
}

impl FormatNodeRule<WithItem> for FormatWithItem {
fn fmt_fields(&self, item: &WithItem, f: &mut PyFormatter) -> FormatResult<()> {
Expand All @@ -28,40 +86,52 @@ impl FormatNodeRule<WithItem> for FormatWithItem {
f.context().source(),
);

// Remove the parentheses of the `with_items` if the with statement adds parentheses
if f.context().node_level().is_parenthesized() {
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)
match self.layout {
// Remove the parentheses of the `with_items` if the with statement adds parentheses
WithItemLayout::ParenthesizedContextManagers => {
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)?;
}
}

WithItemLayout::SingleParenthesizedContextManager => {
write!(
f,
[maybe_parenthesize_expression(
context_expr,
item,
Parenthesize::IfBreaks
)]
)?;
}

WithItemLayout::Python38OrOlder => {
let parenthesize = if is_parenthesized {
Parenthesize::IfBreaks
} else {
Parenthesize::IfRequired
};
write!(
f,
[maybe_parenthesize_expression(
context_expr,
item,
parenthesize
)]
)?;
}
} else {
// 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
)]
)?;
}

if let Some(optional_vars) = optional_vars {
Expand Down

0 comments on commit a56d42f

Please sign in to comment.