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.

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

Formatter undocumented deviation: No wrap in long return #8182

Closed
henribru opened this issue Oct 24, 2023 · 10 comments
Closed

Formatter undocumented deviation: No wrap in long return #8182

henribru opened this issue Oct 24, 2023 · 10 comments
Assignees
Labels
formatter Related to the formatter

Comments

@henribru
Copy link

Black:

def foo():
    if foo:
        return (
            aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
        )

Ruff:

def foo():
    if foo:
        return aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
@MichaReiser MichaReiser added the formatter Related to the formatter label Oct 24, 2023
@MichaReiser MichaReiser added this to the Formatter: Stable milestone Oct 24, 2023
@MichaReiser
Copy link
Member

I'm slightly confused. Black generally tries to avoid parentheses if the parenthesized content also exceeds the line length. We added support for this behavior in #6817 (and refined it later). Now, it seems that this logic doesn't (always?) apply for attribute chains?

@charliermarsh
Copy link
Member

I don't quite understand that either, I wonder if it's intentional.

@njzjz
Copy link

njzjz commented Oct 26, 2023

I got the same behavior with assert.

@MichaReiser MichaReiser self-assigned this Oct 30, 2023
@MichaReiser
Copy link
Member

MichaReiser commented Oct 30, 2023

I played with this further and it is correct. Black always wraps attributes in parentheses, even if it doesn't make them fit.

Black

# fits parenthesized
______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvv
)
return (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbb
)

# exceeds parenthesized
______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvv
)
return (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbb
)


def doesnt_exceed_line_length_when_parenthesizing():
    ______ = (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvv
    )
    return (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbb
    )


def exceeds_line_length_parenthesized():
    ______ = (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvv
    )
    return (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbb
    )

Ruff would remove the parentheses for the "exceeds line length" cases. I believe the same is true for call chains.

Changing our code to use the normal Multiline layout for attribute chains in itself isn't too hard. However, BestFit and `Multiline don't just differ in when they set parentheses, they also use different breaking precedences:

  • BestFitParentheses: Breaks right first. It first breaks the value (right) before parenthesizing the target (left). Similar to BestFitting
  • Multiline: Breaks the left first: It breaks the target (left) before parenthesizing the value (right). Normal group sequence.

Changing the precedence between breaking the target or value has the result that:

state[
    "event_queue_longpoll_timeout_seconds"
] = settings.EVENT_QUEUE_LONGPOLL_TIMEOUT_SECONDS

gets reformatted to

state["event_queue_longpoll_timeout_seconds"] = (
    settings.EVENT_QUEUE_LONGPOLL_TIMEOUT_SECONDS
)

which matches Black's preview style, at least for attributes. I believe implementing the preview style requires changing BestFit to be right to left too, but that seems straightforward.

Changing the precedence regresses our similarity index significantly and I don't know of a good way to make our parenthesizing left to right. The way to achieve this is by:

  • Either use BestFitting which will be slower and requires another MaybeParenthesize layout which I would prefer to aovid.
  • Change the group hierarchy so that the target expression's group encloses the value. This is challenging because the left could be any expression.

I don't think it's worth spending time fixing this now, considering that implementing preview style "solves" the problem and it seems rare enough (I didn't find any usages in our baseline projects). Arguably, Ruff's behavior is also more consistent because it uses it for all expressions that are "unsplittable" whereas Black only uses it for some unsplittable expressions.

@charliermarsh I propose that we document this as know and intentional deviation.

@MichaReiser
Copy link
Member

I opened an issue on the Black repository to see if omitting parentheses is intentional. psf/black#4001

@MichaReiser
Copy link
Member

Black considers aligning with ruff's style.

@MichaReiser
Copy link
Member

I recommend us to keep it as for now and see if Black changes their style.

@MichaReiser MichaReiser closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2023
@henribru
Copy link
Author

henribru commented Nov 2, 2023

Can it get added to the documented deviations in the meantime?

Edit: I see now that you already suggested that earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

No branches or pull requests

4 participants