Keep user inserted optional parentheses

[cdce8p]

This is the followup to #2163 as it was seen as a noticeable improvement but rejected due to the large impact on existing codebases.

With this PR, black will now keep user-inserted optional parentheses if at least 1 delimiter with LOGIC_PRIORITY or higher is present. This would allow developers the option to actively choose the improved style while not changing existing codebases.

If the idea itself is excepted, we could also discuss if this change should apply to all delimiters.

Example style

        return subscript_start is not None and any(
            n.type in TEST_DESCENDANTS for n in subscript_start.pre_order()
        )

# OR
        return (
            subscript_start is not None
            and any(n.type in TEST_DESCENDANTS for n in subscript_start.pre_order())
        )

For more examples, please refer to the original PR #2163 or the issue #2156

Closes: #2156

/CC: @ambv, @JelleZijlstra, @cooperlees, @felix-hilden As you all have in some form participated in the original discussion, I'm especially interested in your thoughts.

Remaining tasks

• Decide if the PR should move forward
• [Decide if change should apply to all delimiters]
• Fix failing tests (if any)
• Add new test case
• Add changelog entry

[felix-hilden]

I think the consensus was that it's a good style, and in my mind it also extends to other places like you've pointed out. But what's up for contention is whether or not the functionality to keep existing formatting should be added. If I'm not mistaken, this would be akin to the magic trailing comma. I think it would be neat to have, but the maintainers may disagree.

[ambv]

The reason I'm hesitant to merge this is that this introduces a weird dependency between runs in Black:

• Black leaves existing parentheses;
• but sometimes it puts parentheses by itself;
• those same parentheses are now treated as pre-existing on another run of the tool.

We had that same thing happen with the magic trailing commas and it introduced a long-running formatting instability (e.g. second run of the tool would reformat differently than the first run). Looking at the changes here, it looks a whole lot like this other case we fought for a long time.

[cdce8p]

I understand the concern. The question would be is there a solution to make it work? There is the --skip-magic-trailing-comma option, maybe we could add a --skip-existing-parentheses one?

Obviously, I wouldn't have opened the initial issue and the PRs if I didn't believe it would be a useful addition to black and from the feedback so far, it seems that everybody agrees.

[pradyunsg]

I understand (and agree with) @ambv's concerns regarding similarity with the trailing comma situation. On the other hand, as someone who finds the final behaviour of trailing commas to be pretty great, I think this will similarly be a great improvement albeit with those pesky transition costs. There's some flavour of "how to reduce transition costs" in #2394.

Looking at the changes here, it looks a whole lot like this other case we fought for a long time.

So... does this mean that:

• this shouldn't be pursued?
• this will need a bunch more energy and effort that is expected to be worth it?
• more discussion is necessary to determine something we don't know yet?

As someone interested in this as a user and a potential code contributor, the next steps here seem very unclear to me. What's the next thing to do here to help get this to a decision of what-to-do-here?

[cdce8p]

@pradyunsg Personally, I would still like to see this move forward. However I feel that at the moment this would be a waist of my time, as it's not yet clear if something like this would ever be accepted. The current PR is enough to illustrate the point IMO.

@felix-hilden wrote an excellent comment regarding that topic: #2156 (comment). Unfortunately, there haven't been any responses yet.

[Akuli]

Can you please make this available under an --experimental-foo flag? The current behaviour sucks, to be honest.

            newline_bytes = self.settings.get("line_ending", settings.LineEnding).value.encode(
                "ascii"
            )

[cdce8p]

I've marked the PR to as draft today. Unfortunately, I won't have time to work on it anytime soon. If anybody wants to pick this up and explore if it makes sense with an experimental flag, please feel free to do so!

[felix-hilden]

Honestly, if we start working on this feature I'd much rather it not be dependent on previous formatting. But this PR could be a good start for the implementation! It could also fit nicely under the --preview flag (#2752). But we still need some discussion in the original issue. Many thanks for the work you've already put in!

[cdce8p]

It could also fit nicely under the --preview flag (#2752). But we still need some discussion in the original issue. Many thanks for the work you've already put in!

Absolutely. If the current pain points get resolved, I imaging this might even be better than the current style. Still lots to discuss though. I would love it if black get's there some day.

[JelleZijlstra]

Unfortunately this has been stuck for over a year and has various merge conflicts. I'm also hesitant to increase our dependency on previous formatting. I'm going to close this for now, but we can consider in a new PR whether to make a similar change in the preview style.