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

Jump to bottom

Expand if-expressions with leading and trailing comments #7082

Closed
wants to merge 2 commits into from
Closed

Expand if-expressions with leading and trailing comments #7082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcab12b
Expand if-exp
charliermarsh Sep 3, 2023
7a6fb92
Require brackets
charliermarsh Sep 3, 2023
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
58 changes: 58 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/if.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,61 @@ def something():
if flat
else ValuesListIterable)
)


# Expanding leading and trailing comments.
before = (
# comment
0
if self.thing is None
else before - after
)

before = (
0
if self.thing is None
else before - after
# comment
)

before = ( # comment
0
if self.thing is None
else before - after
)

before = (
# comment
(0
if self.thing is None
else before - after
))

before = [
# comment
0
if self.thing is None
else before - after
]

before = (
# comment
0
if self.thing is None
else before - after,
2
)

before = [
# comment
0
if self.thing is None
else before - after,
2
]

before = (
0
if self.thing is None
else before - after # comment
)
55 changes: 51 additions & 4 deletions crates/ruff_python_formatter/src/expression/expr_if_exp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use ruff_formatter::{write, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{Expr, ExprIfExp};
use ruff_python_ast::{Expr, ExprIfExp, ExpressionRef};
use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::Ranged;

use crate::comments::leading_comments;
use crate::expression::parentheses::{
Expand Down Expand Up @@ -53,9 +55,25 @@ impl FormatNodeRule<ExprIfExp> for FormatExprIfExp {
let comments = f.context().comments().clone();

let inner = format_with(|f: &mut PyFormatter| {
// We place `if test` and `else orelse` on a single line, so the `test` and `orelse` leading
// comments go on the line before the `if` or `else` instead of directly ahead `test` or
// `orelse`
// If the expression has any leading or trailing comments, and is "alone" in brackets,
// always expand it, as in:
// ```
// [
// # comment
// 0
// if self.thing is None
// else before - after
// ]
// ```
if (comments.has_leading(item) || comments.has_trailing(item))
&& is_expression_bracketed(item.into(), f.context().source())
Copy link
Member

Choose a reason for hiding this comment

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

We already track whether we are inside of parentheses in NodeLevel

Suggested change
&& is_expression_bracketed(item.into(), f.context().source())
&& f.context().node_level().is_parenthesized()

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 this is quite the same — the condition here is “parenthesized or the only item in a set of brackets (e.g., the only item in a tuple literal or list literal or call).”

{
expand_parent().fmt(f)?;
}

// We place `if test` and `else orelse` on a single line, so the `test` and `orelse`
// leading comments go on the line before the `if` or `else` instead of directly ahead
// `test` or `orelse`.
write!(
f,
[
Expand Down Expand Up @@ -109,3 +127,32 @@ impl Format<PyFormatContext<'_>> for FormatOrElse<'_> {
}
}
}

/// Returns `true` if the expression is surrounded by brackets (e.g., parenthesized, or the only
/// expression in a list, tuple, or set).
fn is_expression_bracketed(expr: ExpressionRef, contents: &str) -> bool {
let mut tokenizer = SimpleTokenizer::starts_at(expr.end(), contents)
.skip_trivia()
.skip_while(|token| matches!(token.kind, SimpleTokenKind::Comma));

if matches!(
tokenizer.next(),
Some(SimpleToken {
kind: SimpleTokenKind::RParen | SimpleTokenKind::RBrace | SimpleTokenKind::RBracket,
..
})
) {
let mut tokenizer =
SimpleTokenizer::up_to_without_back_comment(expr.start(), contents).skip_trivia();

matches!(
tokenizer.next_back(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen | SimpleTokenKind::LBrace | SimpleTokenKind::LBracket,
..
})
)
} else {
false
}
Comment on lines +131 to +157
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns `true` if the expression is surrounded by brackets (e.g., parenthesized, or the only
/// expression in a list, tuple, or set).
fn is_expression_bracketed(expr: ExpressionRef, contents: &str) -> bool {
let mut tokenizer = SimpleTokenizer::starts_at(expr.end(), contents)
.skip_trivia()
.skip_while(|token| matches!(token.kind, SimpleTokenKind::Comma));
if matches!(
tokenizer.next(),
Some(SimpleToken {
kind: SimpleTokenKind::RParen | SimpleTokenKind::RBrace | SimpleTokenKind::RBracket,
..
})
) {
let mut tokenizer =
SimpleTokenizer::up_to_without_back_comment(expr.start(), contents).skip_trivia();
matches!(
tokenizer.next_back(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen | SimpleTokenKind::LBrace | SimpleTokenKind::LBracket,
..
})
)
} else {
false
}

}
112 changes: 112 additions & 0 deletions crates/ruff_python_formatter/tests/snapshots/format@expression__if.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,64 @@ def something():
if flat
else ValuesListIterable)
)


# Expanding leading and trailing comments.
before = (
# comment
0
if self.thing is None
else before - after
)

before = (
0
if self.thing is None
else before - after
# comment
)

before = ( # comment
0
if self.thing is None
else before - after
)

before = (
# comment
(0
if self.thing is None
else before - after
))

before = [
# comment
0
if self.thing is None
else before - after
]

before = (
# comment
0
if self.thing is None
else before - after,
2
)

before = [
# comment
0
if self.thing is None
else before - after,
2
]

before = (
0
if self.thing is None
else before - after # comment
)
```

## Output
Expand Down Expand Up @@ -240,6 +298,60 @@ def something():
else ValuesListIterable
)
)


# Expanding leading and trailing comments.
before = (
# comment
0
if self.thing is None
else before - after
)

before = (
0
if self.thing is None
else before - after
# comment
)

before = ( # comment
0
if self.thing is None
else before - after
)

before = (
# comment
0
if self.thing is None
else before - after
)

before = [
# comment
0
if self.thing is None
else before - after
]

before = (
# comment
0 if self.thing is None else before - after,
2,
)

before = [
# comment
0 if self.thing is None else before - after,
2,
]

before = (
0
if self.thing is None
else before - after # comment
)
```


Expand Down
Loading