Extend use of optional parentheses

[cdce8p]

For statements with just one priority delimiter it's often better / easier to read if optional parentheses are inserted. Some examples (from black itself):

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

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

        if id(line.leaves[string_idx]) in line.comments and contains_pragma_comment(
            line.comments[id(line.leaves[string_idx])]
        )

# new
        if (
            id(line.leaves[string_idx]) in line.comments
            and contains_pragma_comment(line.comments[id(line.leaves[string_idx])])
        ):

Additional examples can be seen here: 74b47c1

This is already the default if at least two delimiters with the same max priority are present:

black/src/black/__init__.py

Lines 6760 to 6763 in b39999d

	max_priority = bt.max_delimiter_priority()
	if bt.delimiter_count_with_priority(max_priority) > 1:
	# With more than one delimiter of a kind the optional parentheses read better.
	return False

# example formatting
return (
    is_postponed_evaluation_enabled(node)
    and value.qname() in SUBSCRIPTABLE_CLASSES_PEP585
    and is_node_in_type_annotation_context(node)
)

Notes

The only code change is the first commit: 4a4a3bc

Closes #2156

Remaining Todo's

• Fix broken tests
• Write changelog entry

[ichard26]

Hi @cdce8p,

I see you're a new contributor here, welcome! I took a quick look at your changes and I really like the new output. I probably won't be reviewing this (not qualified to do so yet) but I just wanted to provide my support for the formatting changes. Although unfortunately this does seem like the type of change that would be rejected for being too disruptive.

[JelleZijlstra]

I like this change too, but it's unfortunately a pretty big change.

[JelleZijlstra]

This interacts a bit poorly with the magic trailing comma; we could have put this all in one line.

[cdce8p]

I would like to address those things once we have decided how to best continue with this PR.

[cdce8p]

I like this change too, but it's unfortunately a pretty big change.

I saw that too. As you might have noticed, the required code change itself isn't that big which is why I though a PR would be a good starting point for a discussion if it should be persuade any further. It also has the added benefit that anyone can try it for him-/herself.

The question now becomes how to best continue with it. The way I see it there are a few options, some better suited then others:

1. Implement it as is. This will almost definitely be quite disruptive, although the output is almost always at least equally readable if not clearly better. I first noticed this issue while working on pylint. Most if statements there tend to use isinstance which are often difficult to read with the current formatting. This change is a clear improvement in that regard. pylint-dev/pylint@559d778
2. Leave it as is and close PR because it's too disruptive.
3. Add feature flag to disable/enable it. In case of disable, there is already some president with --skip-string-normalization and --skip-magic-trailing-comma. For enable, it would allow the use of it with the drawback that most will probably never even know about it.
4. Instead of the current change, add logic to keep user inserted optional parentheses. That way a developer does at least have the option to use them and improve the formatting. However, that also means that they would need to be inserted manually each time.

Tbh I'm not sure what to do and in the end it's not my decision to make. Those are just some things I could think of and maybe there is also another way I haven't mentioned. If so, I'm eager to hear about it.

[cdce8p]

As a side node, I just started to look at the result of the Primer test and, at least IMO, almost all the changes appear to be clear improvements. https://github.com/psf/black/pull/2163/checks?check_run_id=2461939530#step:5:42

[cdce8p]

@JelleZijlstra What would be your opinion on how to continue with it?

[JelleZijlstra]

I vote to merge, but I'd like to hear what other maintainers think.

[cooperlees]

I prefer this and think it makes us more consistent. So possibly worth the disruption to codebases.

Wonder if it's worth a community vote going forward with invasive changes?

[cdce8p]

I vote to merge, but I'd like to hear what other maintainers think.

I prefer this and think it makes us more consistent. So possibly worth the disruption to codebases.

👍🏻 I've gone ahead and added a changelog entry as well as fixed the remaining tests. Should be ready for review then

[ambv]

No, I don't think you realize how disruptive of a formatting change this is so late in the game. When I originally made optional parentheses, I added the rule to only use them when there's more than one operator inside because even then this produced too many spurious parens, ballooning the number of lines in sometimes rather simple expressions.

Python doesn't require parens around if statements and so most people don't write them. When you write an expression which suddenly becomes too long, the most natural reaction is to cut the line where you are, and this is how most pre-existing codebases looked like when I was making this functionality in the first place.

There cannot be a flag for it since all our flags disable behavior. They're not for choosing which out of two mutually exclusive approaches to follow. Hence we don't have a "singe quote" option, etc.

Thanks for your contribution, I realize this would have made Black more internally consistent but I'm afraid this would really cause massive uproar for relatively little gain. I understand this is disappointing and nobody is disappointed in this as much as I am, but I believe we are too late in the project to make sweeping changes of this magnitude.

[cdce8p]

@ambv I appreciate the feedback. Although I don't fully agree, I get your point.

Since everyone so far agreed that the style itself is an improvement and consistent with the current one, would it at least be an option to keep user inserted parentheses in that case? That way we could use them when it makes sense without black overwriting the decision.