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

Formatting one-tuples as multi-line if already multi-line #3918

Open
bluetech opened this issue Oct 3, 2023 · 6 comments
Open

Formatting one-tuples as multi-line if already multi-line #3918

bluetech opened this issue Oct 3, 2023 · 6 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@bluetech
Copy link
Contributor

bluetech commented Oct 3, 2023

Describe the style change

Black excludes one-tuples (1,) and single-item subscripts with trailing comma tuple[int,] from magic comma handling, because unlike in list literals etc. the comma here is required, so cannot be used to distinguish between the user's desire to single-line or multi-line.

The single-line format chosen by Black is the desired behavior for "actual" one-tuples, but is not the desired behavior for "currently 1 item but maybe more in the future" tuples.

Examples in the current Black style

Given input:

class WidgetAdmin(admin.ModelAdmin):
    readonly_fields = (
        'id',
    )
    fields = ('foo',)

the formatting is:

class WidgetAdmin(admin.ModelAdmin):
    readonly_fields = ("id",)
    fields = ("foo",)

Desired style

I would like Black to have a special case for one-tuples (and one-subscripts), distinguishing between the newline and no-newline cases. Black would use the multiline format if the input is multiline.

class WidgetAdmin(admin.ModelAdmin):
    readonly_fields = (
        "id",
    )
    fields = ("foo",)

This adds a new form of context sensitivity in addition to magic trailing comma, but I think it makes sense since it plugs a hole in the magic trailing comma handling.

Additional context

Working on moving some large projects to use Black, this is my major gripe. The problem with the current style is that it removes the git-diff friendliness of the multi-line format, which magic trailing comma normally handles nicely.

Common examples for us are in Django Admin classes (like above), another is in Literals, but we have them in a lot of different cases.

Known workarounds:

  1. Switch to list literals. Not great because lists are mutable, heavier than tuples, and sometimes a tuple specifically is needed.

  2. Add a "forcing" comment:

        readonly_fields = (
           "id",
            #
       )

    I think it's not very pretty.

Backward compat: it breaks compat in the sense that arbitrary input will get different output. But given already-formatted input, the output isn't changed. This adheres to the Stability Policy if I understand it correctly.

@bluetech bluetech added the T: style What do we want Blackened code to look like? label Oct 3, 2023
@bluetech
Copy link
Contributor Author

I looked into this briefly. AFAICT, the main problem with implementing this is that the AST does not preserve non-logical newlines, and there is no way to go from an AST node to the original tokens either. The NL token necessary for detecting this case is gone. So implementing this proposal requires changes to blib2to3.

@JelleZijlstra
Copy link
Collaborator

the main problem with implementing this is that the AST does not preserve non-logical newlines

That shouldn't matter. Black doesn't use the AST in its formatting logic, only in the safety check. The formatting logic uses blib2to3's CST, which does preserve newlines.

@bluetech
Copy link
Contributor Author

Thanks @JelleZijlstra, if you don't mind can you point to me how to get at the newlines? Given the following code

(
    999,
)

the result of lib2to3_parse is the following (at least according to the __repr__), which is the same as for (999,):

Node(file_input, [
    Node(simple_stmt, [
        Node(atom, [
            Leaf(LPAR, '('),
            Node(testlist_gexp, [
                Leaf(NUMBER, '999'),
                Leaf(COMMA, ','),
            ]),
            Leaf(RPAR, ')'),
        ]),
        Leaf(NEWLINE, '\n'),
    ]),
    Leaf(ENDMARKER, ''),
])

@JelleZijlstra
Copy link
Collaborator

I believe you have to look at node.prefix, which isn't in the repr. Black's debug.py includes this information.

@JelleZijlstra
Copy link
Collaborator

I should say that I don't think I like this proposed change. I generally prefer Black to take existing formatting into account as little as possible, so that Blackened code looks consistent. I know the magic trailing comma is an exception to that rule, but that doesn't mean we have to add more exceptions.

I do see why you want a special case here, and if there's widespread support I won't block this change.

@bluetech
Copy link
Contributor Author

bluetech commented Nov 4, 2023

The reason I'm pursuing this is that I'm working on formatting our internal code with Black, but this pattern occurs a few hundred times and I prefer not to turn them all into single lines. So I'll try to complete a proof-of-concept at least for evaluation purposes, but I'll completely understand if the proposal ends up being rejected for the reasons you've stated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

2 participants