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: Attribute and call chains "fluent interface" #5343

Closed
Tracked by #6069 ...
konstin opened this issue Jun 23, 2023 · 2 comments · Fixed by #6151
Closed
Tracked by #6069 ...

Formatter: Attribute and call chains "fluent interface" #5343

konstin opened this issue Jun 23, 2023 · 2 comments · Fixed by #6151
Assignees
Labels
formatter Related to the formatter

Comments

@konstin
Copy link
Member

konstin commented Jun 23, 2023

Black supports attribute access and call chaining to provide good formatting for fluent interface style programming with extensive method chaining which is common e.g. with django (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains):

def example(session):
    result = (
        session.query(models.Customer.id)
        .filter(
            models.Customer.account_id == account_id,
            models.Customer.email == email_address,
        )
        .order_by(models.Customer.id.asc())
        .all()
    )

#5340 implements a part of this badly to fix a bug and #5341 adds basic function call formatting.

We need to implement a formatting where the outermost call adds the optional parentheses and formats all inner accesses and calls to not have them add parentheses. A complication is that unlike for if-statements the tree is left recursive, meaning that for a.b().c.d.e the outermost call see an access of e on a.b().c.d, while we must format first a, then .b(), .d and finally .e. We can implement by either collecting into a stack (needs allocation) or using recursing (we most not overrun the recursion and stack limits).

@konstin konstin added the formatter Related to the formatter label Jun 23, 2023
@MichaReiser MichaReiser added this to the Formatter: Alpha milestone Jul 31, 2023
konstin added a commit that referenced this issue Aug 4, 2023
Implement fluent style/call chains. See the `call_chains.py` formatting
for examples.

This isn't fully like black because in `raise A from B` they allow `A`
breaking can influence the formatting of `B` even if it is already
multiline.

Similarity index:

| project      | main  | PR    |
|--------------|-------|-------|
| build        | ???   | 0.753 |
| django       | 0.991 | 0.998 |
| transformers | 0.993 | 0.994 |
| typeshed     | 0.723 | 0.723 |
| warehouse    | 0.978 | 0.994 |
| zulip        | 0.992 | 0.994 |

Call chain formatting is affected by
#627, but i'm cutting scope
here.

Closes #5343

**Test Plan**:
 * Added a dedicated call chains test file
 * The ecosystem checks found some bugs
 * I manually check django and zulip formatting

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
@olejorgenb
Copy link

Probably wrong place to comment on this, but I don't think black's approach here is sufficient. It only work when the chain is very long, and for fluent definitions you usually want one call per line regardless of the number of calls...

Personally I think a OK compromise would be to use explicit parenthesis around a call-chain as a marker that it should be formatted with one line per call?

@MichaReiser
Copy link
Member

Hi @olejorgenb There's an ongoing discussion about this in #8598

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

Successfully merging a pull request may close this issue.

3 participants