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

Respect parenthesized generators in has_own_parentheses #8100

Merged
merged 1 commit into from Oct 22, 2023

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Oct 20, 2023

Summary

When analyzing:

if "root" not in (
    long_tree_name_tree.split("/")[0]
    for long_tree_name_tree in really_really_long_variable_name
):
    msg = "Could not find root. Please try a different forest."
    raise ValueError(msg)

We missed that the generator expression is parenthesized, because the parentheses are part of the generator -- so is_expression_parenthesized returns False. We needed to take into account that generators and tuples may or may not be parenthesized when determining whether we can omit parentheses while splitting an expression.

Closes #8090.

Test Plan

No changes in similarity.

Before:

project similarity index total files changed files
cpython 0.75803 1799 1647
django 0.99983 2772 34
home-assistant 0.99953 10596 186
poetry 0.99891 317 17
transformers 0.99966 2657 330
twine 1.00000 33 0
typeshed 0.99978 3669 20
warehouse 0.99977 654 13
zulip 0.99970 1459 22

After:

project similarity index total files changed files
cpython 0.75803 1799 1647
django 0.99983 2772 34
home-assistant 0.99953 10596 186
poetry 0.99891 317 17
transformers 0.99966 2657 330
twine 1.00000 33 0
typeshed 0.99978 3669 20
warehouse 0.99977 654 13
zulip 0.99970 1459 22

@@ -224,7 +224,7 @@ impl NeedsParentheses for ExprTuple {
/// Return `true` if a tuple is parenthesized in the source code.
pub(crate) fn is_tuple_parenthesized(tuple: &ExprTuple, source: &str) -> bool {
let Some(elt) = tuple.elts.first() else {
return false;
return true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty tuple has to be parenthesized. Otherwise, it... can't exist?

@@ -510,7 +512,7 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool

if visitor.max_precedence == OperatorPrecedence::None {
true
} else if visitor.pax_precedence_count > 1 {
} else if visitor.max_precedence_count > 1 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a typo, but please correct me if I'm wrong.

@charliermarsh charliermarsh added bug Something isn't working formatter Related to the formatter labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Black deviation with if x in some comprehension
2 participants