Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
| + )(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( | ||
| + User.created_at.desc() | ||
| + ).with_for_update(key_share=True).all() | ||
| + filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( |
There was a problem hiding this comment.
Requires call chain formatting
| for ( | ||
| z | ||
| ) in (NOT_YET_IMPLEMENTED_generator_key for NOT_YET_IMPLEMENTED_generator_key in []): | ||
| for z in (NOT_YET_IMPLEMENTED_generator_key for NOT_YET_IMPLEMENTED_generator_key in []): |
There was a problem hiding this comment.
Requires proper generator formatting
| "Let's" "start" "with" "a" "simple" "example" | ||
|
|
||
| "Let's" "start" "with" "a" "simple" "example" "now repeat after me:" "I am confident" "I am confident" "I am confident" "I am confident" "I am confident" | ||
| ( |
There was a problem hiding this comment.
This per se is a regression because black keeps the string flat inside of ExprStmt. However, this is the same as that black does not try to wrap binary expressions on statement level (except if they contain a list). I decided to remove the work around for strings so that we have the same behavior for all expressions and either solve it holistically or not at all.
There was a problem hiding this comment.
implicit string concatenation (vs. triple quoted strings) at statement level seems like an odd choice, do you by chance have any usage examples?
There was a problem hiding this comment.
I didn't find any in the whole django project... (as expected?)
| _parent: AnyNodeRef, | ||
| _context: &PyFormatContext, | ||
| ) -> OptionalParentheses { | ||
| OptionalParentheses::Never |
There was a problem hiding this comment.
Names never require optional parentheses... There's nothing to split by
| Parentheses::Optional => Parentheses::Never, | ||
| parentheses => parentheses, | ||
| ) -> OptionalParentheses { | ||
| self.func.needs_parentheses(parent, context) |
There was a problem hiding this comment.
Never is incorrect. It requires parentheses if the func node requires parentheses (see attribute chain)
There was a problem hiding this comment.
yep i should have placed a todo comment about missing fluent style support there
ceb3a94 to
a3145c1
Compare
a3145c1 to
f1b9008
Compare
f1b9008 to
393b0b2
Compare
| if self.value.is_str() { | ||
| let contents = context.locator().slice(self.range()); | ||
| // Don't wrap triple quoted strings | ||
| if is_multiline_string(self, context.source()) || !is_implicit_concatenation(contents) { |
There was a problem hiding this comment.
This is somewhat slow. We should consider extracting the ranges of all multiline strings (similar to Ruff)
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
| Parentheses::Optional => Parentheses::Never, | ||
| parentheses => parentheses, | ||
| ) -> OptionalParentheses { | ||
| self.func.needs_parentheses(parent, context) |
There was a problem hiding this comment.
yep i should have placed a todo comment about missing fluent style support there
| + )(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( | ||
| + User.created_at.desc() | ||
| + ).with_for_update(key_share=True).all() | ||
| + filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by( |
| "Let's" "start" "with" "a" "simple" "example" | ||
|
|
||
| "Let's" "start" "with" "a" "simple" "example" "now repeat after me:" "I am confident" "I am confident" "I am confident" "I am confident" "I am confident" | ||
| ( |
There was a problem hiding this comment.
implicit string concatenation (vs. triple quoted strings) at statement level seems like an odd choice, do you by chance have any usage examples?
| _context: &PyFormatContext, | ||
| ) -> OptionalParentheses { | ||
| // Unlike tuples, named expression parentheses are not part of the range even when | ||
| // mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details. |
There was a problem hiding this comment.
it's actually a TODO on my end
| // mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details. | |
| // mandatory. TODO(konstin): Implement [PEP 572](https://peps.python.org/pep-0572/) rules. |
393b0b2 to
02d81a9
Compare
02d81a9 to
89885ca
Compare
Summary
This PR refactors
NeedsParenthesesso that implementations get access to theparentnode. Having access to the parent node is e.g. required for omitting parentheses from the walrus operator when not strictly necessary.The main challenge that I ran into is that simply storing the parent node in
Parenthesize::Optionalisn't possible becauseParenthesizethen requires lifetimes, andAsFormat(withFormatRuleWithOptions) does not support lifetimes... ughThis forced me to rethink the problem and I'm not entirely unhappy with the outcome (There's room for improvement on the naming).
FormatExpr: Accepts aParenthesesoption that can either beAlways,Never,Preserve, wherePreserveis the default. This covers the base case of expression formattingmaybe_parenthesize_expressionis a new builder that may parenthesize an expression depending onParenthesizedand the expressionsNeedsParenthesesimplementation.I went along and removed
Parentheses::Customas part of this PR. This caused me some headaches with string formatting until I realized that we currently make an exception for string formatting which we do not for other nodes. The old implementation forced strings to be flat when used in an Expression statement to match Black, but we don't enforce the same for binary expressions and other expressions nodes. My take is that we should solve this holistically rather than for some nodes only.Test Plan
Most of this is pure refactor. I reviewed the snapshot tests and they seem reasonable.