Skip to content

Commit

Permalink
Maybe parenthesize long constants and names
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Aug 23, 2023
1 parent 3fa14d5 commit d50042b
Show file tree
Hide file tree
Showing 18 changed files with 312 additions and 356 deletions.
4 changes: 2 additions & 2 deletions crates/ruff_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2534,17 +2534,17 @@ impl<'a, Context> BestFitting<'a, Context> {

impl<Context> Format<Context> for BestFitting<'_, Context> {
fn fmt(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
let mut buffer = VecBuffer::new(f.state_mut());
let variants = self.variants.items();

let mut formatted_variants = Vec::with_capacity(variants.len());

for variant in variants {
let mut buffer = VecBuffer::with_capacity(8, f.state_mut());
buffer.write_element(FormatElement::Tag(StartEntry));
buffer.write_fmt(Arguments::from(variant))?;
buffer.write_element(FormatElement::Tag(EndEntry));

formatted_variants.push(buffer.take_vec().into_boxed_slice());
formatted_variants.push(buffer.into_vec().into_boxed_slice());
}

// SAFETY: The constructor guarantees that there are always at least two variants. It's, therefore,
Expand Down
8 changes: 6 additions & 2 deletions crates/ruff_formatter/src/format_element/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,12 @@ impl Document {
expands
}

let mut enclosing: Vec<Enclosing> = Vec::new();
let mut interned: FxHashMap<&Interned, bool> = FxHashMap::default();
let mut enclosing = Vec::with_capacity(if self.is_empty() {
0
} else {
self.len().ilog2() as usize
});
let mut interned = FxHashMap::default();
propagate_expands(self, &mut enclosing, &mut interned);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,9 @@
# Regression: https://github.com/astral-sh/ruff/issues/5338
if a and not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
...

if (
not
# comment
a):
...
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
x = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

x_aa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
xxxxx = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

while (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
):
pass

while aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
pass

# Only applies in `Parenthesize::IfBreaks` positions
raise aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

raise (
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
)

raise a from aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
raise a from aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

# Can never apply on expression statement level
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

# Is it only relevant for items that can't break

aaaaaaa = 111111111111111111111111111111111111111111111111111111111111111111111111111111
aaaaaaa = (
1111111111111111111111111111111111111111111111111111111111111111111111111111111
)

aaaaaaa = """111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""

# Never parenthesize multiline strings
aaaaaaa = (
"""1111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111"""
)



aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbb
aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

aaaaaaaa = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb


for converter in connection.ops.get_db_converters(
expression
) + expression.get_db_converters(connection):
...
5 changes: 4 additions & 1 deletion crates/ruff_python_formatter/src/expression/expr_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ impl NeedsParentheses for ExprCall {
{
OptionalParentheses::Multiline
} else {
self.func.needs_parentheses(self.into(), context)
match self.func.needs_parentheses(self.into(), context) {
OptionalParentheses::BestFit => OptionalParentheses::Never,
parentheses => parentheses,
}
}
}
}
12 changes: 10 additions & 2 deletions crates/ruff_python_formatter/src/expression/expr_constant.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::comments::SourceComment;
use ruff_formatter::FormatRuleWithOptions;
use ruff_formatter::{FormatContext, FormatOptions, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{Constant, ExprConstant, Ranged};
use ruff_text_size::{TextLen, TextRange};
Expand Down Expand Up @@ -86,8 +86,16 @@ impl NeedsParentheses for ExprConstant {
} else {
OptionalParentheses::Multiline
}
} else {
} else if self.value.is_none() | self.value.is_bool() || self.value.is_ellipsis() {
OptionalParentheses::Never
} else {
let text_len = context.source()[self.range].len();

if text_len > 4 && text_len < context.options().line_width().value() as usize {
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
}
}
}
Expand Down
20 changes: 17 additions & 3 deletions crates/ruff_python_formatter/src/expression/expr_f_string.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use super::string::{AnyString, FormatString};
use crate::context::PyFormatContext;

use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::{FormatNodeRule, PyFormatter};
use ruff_formatter::FormatResult;
use ruff_formatter::{FormatContext, FormatOptions, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::ExprFString;

Expand All @@ -20,8 +21,21 @@ impl NeedsParentheses for ExprFString {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Multiline
if self.implicit_concatenated {
OptionalParentheses::Multiline
} else {
let text = &context.source()[self.range];

if !text.contains(['\n', '\r'])
&& text.len() > 4
&& text.len() < context.options().line_width().value() as usize
{
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
}
}
}
11 changes: 8 additions & 3 deletions crates/ruff_python_formatter/src/expression/expr_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::comments::SourceComment;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::FormatNodeRule;
use ruff_formatter::{write, FormatContext};
use ruff_formatter::{write, FormatContext, FormatOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::ExprName;

Expand Down Expand Up @@ -38,9 +38,14 @@ impl NeedsParentheses for ExprName {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
let text_len = context.source()[self.range].len();
if text_len > 4 && text_len < context.options().line_width().value() as usize {
OptionalParentheses::BestFit
} else {
OptionalParentheses::Never
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ impl NeedsParentheses for ExprSubscript {
{
OptionalParentheses::Multiline
} else {
self.value.needs_parentheses(self.into(), context)
match self.value.needs_parentheses(self.into(), context) {
OptionalParentheses::BestFit => OptionalParentheses::Never,
parentheses => parentheses,
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_python_formatter/src/expression/expr_unary_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use ruff_python_ast::{ExprUnaryOp, Ranged};
use ruff_text_size::{TextLen, TextRange};

use ruff_formatter::prelude::{hard_line_break, space, text};
use ruff_formatter::{Format, FormatContext, FormatResult};
use ruff_formatter::{Format, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};

Expand Down Expand Up @@ -57,7 +57,7 @@ impl FormatNodeRule<ExprUnaryOp> for FormatExprUnaryOp {
// a)
// ```
if !leading_operand_comments.is_empty()
&& !is_operand_parenthesized(item, f.context().source_code().as_str())
&& !is_operand_parenthesized(item, f.context().source())
{
hard_line_break().fmt(f)?;
} else if op.is_not() {
Expand Down
37 changes: 36 additions & 1 deletion crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::cmp::Ordering;

use ruff_formatter::{
write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions,
format_args, write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions,
};
use ruff_python_ast as ast;
use ruff_python_ast::node::AnyNodeRef;
Expand Down Expand Up @@ -220,6 +220,40 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
}
}
},
OptionalParentheses::BestFit => match parenthesize {
Parenthesize::IfBreaksOrIfRequired => {
parenthesize_if_expands(&expression.format().with_options(Parentheses::Never))
.fmt(f)
}

Parenthesize::Optional | Parenthesize::IfRequired => {
expression.format().with_options(Parentheses::Never).fmt(f)
}
Parenthesize::IfBreaks => {
let group_id = f.group_id("optional_parentheses");
let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);
let format_expression = expression
.format()
.with_options(Parentheses::Never)
.memoized();

best_fitting![
group(&format_expression).with_group_id(Some(group_id)),
group(&format_args![
text("("),
soft_block_indent(&format_expression),
text(")")
])
.with_group_id(Some(group_id))
.should_expand(true),
group(&format_expression)
.with_group_id(Some(group_id))
.should_expand(true)
]
.with_mode(BestFittingMode::AllLines)
.fmt(f)
}
},
OptionalParentheses::Never => match parenthesize {
Parenthesize::IfBreaksOrIfRequired => {
parenthesize_if_expands(&expression.format().with_options(Parentheses::Never))
Expand All @@ -230,6 +264,7 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
expression.format().with_options(Parentheses::Never).fmt(f)
}
},

OptionalParentheses::Always => {
expression.format().with_options(Parentheses::Always).fmt(f)
}
Expand Down
10 changes: 8 additions & 2 deletions crates/ruff_python_formatter/src/expression/parentheses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ pub(crate) enum OptionalParentheses {
/// Add parentheses if the expression expands over multiple lines
Multiline,

/// Always set parentheses regardless if the expression breaks or if they were
/// Always set parentheses regardless if the expression breaks or if they are
/// present in the source.
Always,

/// Never add parentheses
/// Add parentheses if it helps to make this expression fit. Otherwise never add parentheses.
/// This mode should only be used for expressions that don't have their own split points, e.g. identifiers,
/// or constants.
BestFit,

/// Never add parentheses. Use it for expressions that have their own parentheses or if the expression body always spans multiple lines (multiline strings).
Never,
}

Expand Down Expand Up @@ -48,6 +53,7 @@ pub(crate) enum Parenthesize {
/// Parenthesizes the expression if the group doesn't fit on a line (e.g., even name expressions are parenthesized), or if
/// the expression doesn't break, but _does_ reports that it always requires parentheses in this position (e.g., walrus
/// operators in function return annotations).
// TODO can this be replaced by IfBreaks
IfBreaksOrIfRequired,
}

Expand Down
17 changes: 8 additions & 9 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,10 @@ if True:
#[test]
fn quick_test() {
let src = r#"
@MyDecorator(list = a) # fmt: skip
# trailing comment
class Test:
pass
for converter in connection.ops.get_db_converters(
expression
) + expression.get_db_converters(connection):
...
"#;
// Tokenize once
let mut tokens = Vec::new();
Expand Down Expand Up @@ -291,9 +289,10 @@ class Test:

assert_eq!(
printed.as_code(),
r#"while True:
if something.changed:
do.stuff() # trailing comment
r#"for converter in connection.ops.get_db_converters(
expression
) + expression.get_db_converters(connection):
...
"#
);
}
Expand Down
Loading

0 comments on commit d50042b

Please sign in to comment.