-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix attribute chain own line comments #5340
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
x21 = ( | ||
a | ||
# trailing name own line | ||
.b | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add one example where the outer most attribute chain has leading and trailing comments too?
} else if let Expr::Attribute(expr_attribute) = value.as_ref() { | ||
// We're in a attribute chain (`a.b.c`). The outermost node adds parentheses if | ||
// required, the inner ones don't need them so we skip the `Expr` formatting that | ||
// normally adds the parentheses. | ||
expr_attribute.format().fmt(f)?; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test case where an inner attribute change is parenthesized? I wonder if we need to preserve the parentheses.
37fdcdd
to
8bc2b89
Compare
Motation
Previously,
got formatted as
which is invalid syntax. This fixes that.
Summary
This implements a basic form of attribute chaining (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains) by checking if any inner attribute access contains an own line comment, and if this is the case, adds parentheses around the outermost attribute access while disabling parentheses for all inner attribute expressions. We want to replace this with an implementation that uses recursion or a stack while formatting instead of in
needs_parentheses
and also includes calls rather sooner than later, but i'm fixing this now because i'm uncomfortable with having known invalid syntax generation in the formatter.Test Plan
I added new fixtures.