Skip to content

Add BestFittingMode#5184

Merged
MichaReiser merged 1 commit intomainfrom
best-fitting-mode
Jun 20, 2023
Merged

Add BestFittingMode#5184
MichaReiser merged 1 commit intomainfrom
best-fitting-mode

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 19, 2023 •
edited
Loading

Summary

Black supports for layouts when it comes to breaking binary expressions:

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
enum BinaryLayout {
    /// Put each operand on their own line if either side expands
    Default,

    /// Try to expand the left to make it fit. Add parentheses if the left or right don't fit.
    ///
    ///```python
    /// [
    ///     a,
    ///     b
    /// ] & c
    ///```
    ExpandLeft,

    /// Try to expand the right to make it fix. Add parentheses if the left or right don't fit.
    ///
    /// ```python
    /// a & [
    ///     b,
    ///     c
    /// ]
    /// ```
    ExpandRight,

    /// Both the left and right side can be expanded. Try in the following order:
    /// * expand the right side
    /// * expand the left side
    /// * expand both sides
    ///
    /// to make the expression fit
    ///
    /// ```python
    /// [
    ///     a,
    ///     b
    /// ] & [
    ///     c,
    ///     d
    /// ]
    /// ```
    ExpandRightThenLeft,
}

Our current implementation only handles ExpandRight and Default correctly. ExpandLeft turns out to be surprisingly hard. This PR adds a new BestFittingMode parameter to BestFitting to support ExpandLeft.

There are 3 variants that ExpandLeft must support:

Variant 1: Everything fits on the line (easy)

[a, b] + c

Variant 2: Left breaks, but right fits on the line. Doesn't need parentheses

[
	a,
	b
] + c

Variant 3: The left breaks, but there's still not enough space for the right hand side. Parenthesize the whole expression:

(
	[
		a, 
		b
	]
	+ ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
)

Solving Variant 1 and 2 on their own is straightforward The printer gives us this behavior by nesting right inside of the group of left:

group(&format_args![
	if_group_breaks(&text("(")),
	soft_block_indent(&group(&format_args![
		left, 
		soft_line_break_or_space(), 
		op, 
		space(), 
		group(&right)
	])),
	if_group_breaks(&text(")"))
])

The fundamental problem is that the outer group, which adds the parentheses, always breaks if the left side breaks. That means, we end up with

(
	[
		a,
		b
	] + c
)

which is not what we want (we only want parentheses if the right side doesn't fit).

Okay, so nesting groups don't work because of the outer parentheses. Sequencing groups doesn't work because it results in a right-to-left breaking which is the opposite of what we want.

Could we use best fitting? Almost!

best_fitting![
	// All flat
	format_args![left, space(), op, space(), right],
	// Break left
	format_args!(group(&left).should_expand(true), space(), op, space(), right],
	// Break all
	format_args![
		text("("), 
		block_indent!(&format_args![
			left, 
			hard_line_break(), 
			op,
			space()
			right
		])
	]
]

I hope I managed to write this up correctly. The problem is that the printer never reaches the 3rd variant because the second variant always fits:

  • The group(&left).should_expand(true) changes the group so that all soft_line_breaks are turned into hard line breaks. This is necessary because we want to test if the content fits if we break after the [.
  • Now, the whole idea of best_fitting is that you can pretend that some content fits on the line when it actually does not. The way this works is that the printer only tests if all the content of the variant up to the first line break fits on the line (we insert that line break by using should_expand(true)). The printer doesn't care whether the rest a\n, b\n ] + c all fits on (multiple?) lines.

Why does breaking right work but not breaking the left? The difference is that we can make the decision whether to parenthesis the expression based on the left expression. We can't do this for breaking left because the decision whether to insert parentheses or not would depend on a lookahead: will the right side break. We simply don't know this yet when printing the parentheses (it would work for the right parentheses but not for the left and indent).

What we kind of want here is to tell the printer: Look, what comes here may or may not fit on a single line but we don't care. Simply test that what comes after fits on a line.

This PR adds a new BestFittingMode that has a new AllLines option that gives us the desired behavior of testing all content and not just up to the first line break.

Test Plan

I added a new example to BestFitting::with_mode

@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 19, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser requested a review from konstin June 19, 2023 15:53
@MichaReiser MichaReiser added the formatter Related to the formatter label Jun 19, 2023
This was referenced Jun 19, 2023
Merged
Merged
@MichaReiser MichaReiser changed the title Add BeestFittingMode Add BestFittingMode Jun 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2023 •
edited
Loading

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.6±0.01ms     5.3 MB/sec    1.00      7.6±0.01ms     5.4 MB/sec
formatter/numpy/ctypeslib.py               1.00   1586.0±1.64µs    10.5 MB/sec    1.00   1587.6±4.07µs    10.5 MB/sec
formatter/numpy/globals.py                 1.01    152.9±0.28µs    19.3 MB/sec    1.00    152.0±1.21µs    19.4 MB/sec
formatter/pydantic/types.py                1.01      3.1±0.01ms     8.2 MB/sec    1.00      3.1±0.01ms     8.2 MB/sec
linter/all-rules/large/dataset.py          1.00     15.8±0.07ms     2.6 MB/sec    1.04     16.5±0.06ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.9±0.01ms     4.2 MB/sec    1.02      4.0±0.01ms     4.2 MB/sec
linter/all-rules/numpy/globals.py          1.01    513.9±0.96µs     5.7 MB/sec    1.00    507.5±1.10µs     5.8 MB/sec
linter/all-rules/pydantic/types.py         1.01      7.0±0.04ms     3.6 MB/sec    1.00      7.0±0.02ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      7.9±0.02ms     5.1 MB/sec    1.07      8.5±0.37ms     4.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1747.5±6.62µs     9.5 MB/sec    1.04   1814.8±5.56µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    199.8±0.47µs    14.8 MB/sec    1.02    204.7±6.16µs    14.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.01ms     7.0 MB/sec    1.03      3.8±0.01ms     6.8 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.7±0.12ms     5.3 MB/sec    1.05      8.1±0.07ms     5.0 MB/sec
formatter/numpy/ctypeslib.py               1.00  1570.8±29.13µs    10.6 MB/sec    1.04  1634.8±31.98µs    10.2 MB/sec
formatter/numpy/globals.py                 1.00    155.0±3.34µs    19.0 MB/sec    1.02    157.8±5.36µs    18.7 MB/sec
formatter/pydantic/types.py                1.00      3.1±0.05ms     8.2 MB/sec    1.06      3.3±0.04ms     7.8 MB/sec
linter/all-rules/large/dataset.py          1.01     16.3±0.25ms     2.5 MB/sec    1.00     16.2±0.19ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.1±0.05ms     4.1 MB/sec    1.01      4.1±0.05ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    503.7±6.96µs     5.9 MB/sec    1.00   501.6±14.14µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.0±0.10ms     3.7 MB/sec    1.01      7.0±0.10ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      8.1±0.11ms     5.0 MB/sec    1.07      8.6±0.08ms     4.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1746.5±34.44µs     9.5 MB/sec    1.02  1789.8±25.17µs     9.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    202.4±4.58µs    14.6 MB/sec    1.00    202.0±5.22µs    14.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.04ms     6.9 MB/sec    1.04      3.8±0.06ms     6.6 MB/sec

@MichaReiser MichaReiser marked this pull request as ready for review June 19, 2023 16:06
This was referenced Jun 20, 2023
Merged
Merged
@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 20, 2023

@MichaReiser started a stack merge that includes this pull request via Graphite.

@MichaReiser MichaReiser merged commit d9e59b2 into main Jun 20, 2023
@MichaReiser MichaReiser deleted the best-fitting-mode branch June 20, 2023 16:16
@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 20, 2023

@MichaReiser merged this pull request with Graphite.

@MichaReiser MichaReiser mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@konstin konstin konstin approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaReiser @konstin