Skip to content

Commit

Permalink
Potential fix for overlong parenthesized content
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Sep 18, 2023
1 parent 0346e78 commit 4745612
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 125 deletions.
2 changes: 2 additions & 0 deletions crates/ruff_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2013,6 +2013,8 @@ impl<Context> std::fmt::Debug for IndentIfGroupBreaks<'_, Context> {
}
}

// TODO rename to `AllowOverflow`, `SkipFits`?

/// Changes the definition of *fits* for `content`. Instead of measuring it in *flat*, measure it with
/// all line breaks expanded and test if no line exceeds the line width. The [`FitsExpanded`] acts
/// as a expands boundary similar to best fitting, meaning that a [`hard_line_break`] will not cause the parent group to expand.
Expand Down
66 changes: 41 additions & 25 deletions crates/ruff_formatter/src/printer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> {
// line break should be printed as regular line break
return Ok(Fits::Yes);
}
MeasureMode::AllLines => {
MeasureMode::AllLines | MeasureMode::AllowTextOverflow => {
// Continue measuring on the next line
self.state.line_width = 0;
self.state.pending_indent = args.indention();
Expand Down Expand Up @@ -1247,32 +1247,42 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> {
condition,
propagate_expand,
})) => {
let condition_met = match condition {
Some(condition) => {
let group_mode = match condition.group_id {
Some(group_id) => self.group_modes().get_print_mode(group_id)?,
None => args.mode(),
match args.mode() {
PrintMode::Expanded => {
// As usual, nothing to measure
self.stack.push(TagKind::FitsExpanded, args);
}
PrintMode::Flat => {
let condition_met = match condition {
Some(condition) => {
let group_mode = match condition.group_id {
Some(group_id) => {
self.group_modes().get_print_mode(group_id)?
}
None => args.mode(),
};

condition.mode == group_mode
}
None => true,
};

condition.mode == group_mode
}
None => true,
};
if condition_met {
// Measure in fully expanded mode and allow overflows
self.stack.push(
TagKind::FitsExpanded,
args.with_measure_mode(MeasureMode::AllowTextOverflow)
.with_print_mode(PrintMode::Expanded),
);
} else {
if propagate_expand.get() && args.mode().is_flat() {
return Ok(Fits::No);
}

if condition_met {
// Measure in fully expanded mode.
self.stack.push(
TagKind::FitsExpanded,
args.with_print_mode(PrintMode::Expanded)
.with_measure_mode(MeasureMode::AllLines),
);
} else {
if propagate_expand.get() && args.mode().is_flat() {
return Ok(Fits::No);
// As usual
self.stack.push(TagKind::FitsExpanded, args);
}
}

// As usual
self.stack.push(TagKind::FitsExpanded, args);
}
}

Expand Down Expand Up @@ -1354,7 +1364,7 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> {
}
match args.measure_mode() {
MeasureMode::FirstLine => return Fits::Yes,
MeasureMode::AllLines => {
MeasureMode::AllLines | MeasureMode::AllowTextOverflow => {
self.state.line_width = 0;
continue;
}
Expand All @@ -1370,7 +1380,9 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> {
}
}

if self.state.line_width > self.options().line_width.into() {
if self.state.line_width > self.options().line_width.into()
&& args.measure_mode() != MeasureMode::AllowTextOverflow
{
return Fits::No;
}

Expand Down Expand Up @@ -1473,6 +1485,10 @@ enum MeasureMode {
/// The content only fits if none of the lines exceed the print width. Lines are terminated by either
/// a hard line break or a soft line break in [`PrintMode::Expanded`].
AllLines,

/// Measures all lines and allows lines exceeding the line width. Useful when it only matters
/// whether the content **after** fits.
AllowTextOverflow,
}

impl From<BestFittingMode> for MeasureMode {
Expand Down
36 changes: 19 additions & 17 deletions crates/ruff_python_formatter/src/expression/parentheses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,32 +176,34 @@ impl<'content, 'ast> FormatParenthesized<'content, 'ast> {

impl<'ast> Format<PyFormatContext<'ast>> for FormatParenthesized<'_, 'ast> {
fn fmt(&self, f: &mut Formatter<PyFormatContext<'ast>>) -> FormatResult<()> {
let inner = format_with(|f| {
let current_level = f.context().node_level();

let content = format_with(|f| {
group(&format_args![
token(self.left),
dangling_open_parenthesis_comments(self.comments),
soft_block_indent(&Arguments::from(&self.content)),
token(self.right)
soft_block_indent(&Arguments::from(&self.content))
])
.fmt(f)
});

let current_level = f.context().node_level();
let inner = format_with(|f| {
if let NodeLevel::Expression(Some(group_id)) = current_level {
// Use fits expanded if there's an enclosing group that adds the optional parentheses.
// This ensures that expanding this parenthesized expression does not expand the optional parentheses group.
write!(
f,
[fits_expanded(&content)
.with_condition(Some(Condition::if_group_fits_on_line(group_id)))]
)
} else {
// It's not necessary to wrap the content if it is not inside of an optional_parentheses group.
content.fmt(f)
}
});

let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f);

if let NodeLevel::Expression(Some(group_id)) = current_level {
// Use fits expanded if there's an enclosing group that adds the optional parentheses.
// This ensures that expanding this parenthesized expression does not expand the optional parentheses group.
write!(
f,
[fits_expanded(&inner)
.with_condition(Some(Condition::if_group_fits_on_line(group_id)))]
)
} else {
// It's not necessary to wrap the content if it is not inside of an optional_parentheses group.
write!(f, [inner])
}
write!(f, [token(self.left), inner, token(self.right)])
}
}

Expand Down
16 changes: 10 additions & 6 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,13 +217,17 @@ if True:
#[test]
fn quick_test() {
let src = r#"
def main() -> None:
if True:
if True:
some_very_long_variable_name_abcdefghijk = Foo()
some_very_long_variable_name_abcdefghijk = some_very_long_variable_name_abcdefghijk[
some_very_long_variable_name_abcdefghijk.some_very_long_attribute_name
== "This is a very long string abcdefghijk"
]
if True:
if True:
msg += " " + _(
"Since the role is not mentionable, it will be momentarily made mentionable "
"when announcing a streamalert. Please make sure I have the correct "
"permissions to manage this role, or else members of this role won't receive "
"a notification."
)
"#;
// Tokenize once
Expand Down

This file was deleted.

0 comments on commit 4745612

Please sign in to comment.