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

Avoid parentheszing non-fluent attribute chain exceeding the line-length #4001

Open
MichaReiser opened this issue Oct 30, 2023 · 3 comments
Open
Labels
T: bug Something isn't working

Comments

@MichaReiser
Copy link

MichaReiser commented Oct 30, 2023

I wasn't sure if I should submit a style or bug report for this (or should it be a question?). I'm sorry if I picked the wrong template.

Black implements a neat little optimization for constants and names to avoid wrapping them in parentheses when the parenthesized expression exceeds the line length.

In the following code, Black omits parenthesizing the "exceeds parenthesized" cases because the code exceeds the line length when adding and when omitting the optional parentheses, which is why it favors omitting the parentheses. Except, for attributes...

# fits parenthesized
______ = (
    "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvv"
)
______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvv
)
______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvv
)

# exceeds parenthesized
______ = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvv"
______ = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvvv
______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvvv
)


def doesnt_exceed_line_length_when_parenthesizing():
    ______ = (
        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvv"
    )
    ______ = (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvv
    )
    ______ = (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvv
    )


def exceeds_line_length_parenthesized():
    ______ = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvv"
    ______ = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvvvv
    ______ = (
        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvvvv
    )

I implemented the same behavior in Ruff and incorrectly assumed that it would apply to all expressions that cannot be split further (which includes non-fluent attribute chains). I guess the bug / style suggestion / question is: Should blacked code omit parentheses around attribute chains if they exceed the configured line length regardless if they're parenthesized or not, to be consistent with other unsplittable expressions?

Edit: Somewhat related. Black breaks the left before parenthesizing the right for constants, names, and non-fluent attribute chains.

state[
    "event_queue_longpoll_timeout_seconds"
] = settings.EVENT_QUEUE_LONGPOLL_TIMEOUT_SECONDS

# Instead of, at least without preview mode
state["event_queue_longpoll_timeout_seconds"] = (
    settings.EVENT_QUEUE_LONGPOLL_TIMEOUT_SECONDS
)

but it already breaks the right before the left for binary expressions (and other splittable expressions)

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

It's probably too late to use this as a real argument, considering that this changes with preview style, but handling non-fluent attribute chains identical to constants and names when parenthesizing would make their handling more consistent.

@JelleZijlstra
Copy link
Collaborator

I'd be open to changing Black's behavior here. We probably should actually split these attribute chains, like this:

______ = (
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    .bbbbbbbbbbbbbbbbbbbbbbbbbbvvvvvvvvvvvvvvvvvvv
)

(Similar to how we format fluent interfaces)

@MichaReiser
Copy link
Author

We probably should actually split these attribute chains, like this:

I think that would be ideal. I haven't yet had time to closely look into what it would mean to split attribute chains (how does it affect all other formatting rules). I would love to collaborate on defining a new style together. At least, after getting the preview style implemented. Black's formatting is so good and has many hidden gems you only realize when paying attention. It will take me a while to get up to parity. I appreciate all the hard work you've put into defining the style guide.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 31, 2023

Note that Black currently goes a little out of its way to forbid that kind of split. Even in fluent interfaces, the first attribute access currently remains unsplit. So will require a little bit of figuring out what the right rule is :-)

(Coincidentally, I was just looking at fluent interface formatting to fix some other undesirable splitting caused by experimental string processing and #67 (comment) and #3998).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants