Format assert statement
#5168
Format assert statement
#5168
Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
|
Hmm maybe there's more here. This isn't formatting the same as black. assert (
# Some comment
True # Some comment
# Some comment
), "Some string" # Some comment
# Some comment |
cnpryer
force-pushed
the
stmt-assert
branch
2 times, most recently
from
e03aa97
to
5a9bc05
Compare
There was a problem hiding this comment.
these test cases are copied from black, please add new test cases to crates/ruff_python_formatter/resources/test/fixtures/ruff instead
cnpryer
force-pushed
the
stmt-assert
branch
2 times, most recently
from
04ee3e7
to
d780f2f
Compare
|
Also note if you format this with black # Some assert
assert (
# First inner comment
(1 + 1) # Test's trailing comment
# Below the expression
# Last inner comment
), "Some string" # Message's trailing comment
# Below the assertI reported this here for now. Probably should be a new issue if that one it's mentioned in is not updated. I'll get back to this. |
This reverts commit 239daa7.
|
I find this snapshot result odd, but it could just be my inexperience with |
The snapshots can be hard to read because it shows a diff of the new to old snapshot containing a diff with the black/ruff formatter differences. Overall:
|
|
Thanks for the explanation. Yea I that's tough to understand. Manually reviewing it that result looks fine. Intuitively I would have expected no change in the diff here, but I'm sure I'm just missing something. |
|
This is maybe a better example of what's tripping me up. Manually reviewing the new snapshot ruff and black outputs, and comparing them with the original snapshot, I'd expect there to be no new results displayed during the IIUC this result is displaying the same diff just in a different way. Originally And in this result |
Yeah this is a bit annoying and something that I noticed too. The diff algorithm sometimes gets dripped up and produces changes even if there are non (to this particular section). We could explore if other diffing algorithm help but ultimately the best is to have higher compatibility, which will make the diff algorithm's job easier. |
| if let Some(msg) = msg { | ||
| write!( | ||
| f, | ||
| [ | ||
| text("assert"), | ||
| space(), | ||
| test.format().with_options(Parenthesize::IfBreaks), | ||
| text(","), | ||
| space(), | ||
| msg.format().with_options(Parenthesize::IfBreaks) | ||
| ] | ||
| ) | ||
| } else { | ||
| write!( | ||
| f, | ||
| [ | ||
| text("assert"), | ||
| space(), | ||
| test.format().with_options(Parenthesize::IfBreaks) | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
Nit: I prefer to keep the shared logic outside of the if..else branches. It avoids that the reader has to parse both branches of the if statement to identify what's difference between them
| if let Some(msg) = msg { | |
| write!( | |
| f, | |
| [ | |
| text("assert"), | |
| space(), | |
| test.format().with_options(Parenthesize::IfBreaks), | |
| text(","), | |
| space(), | |
| msg.format().with_options(Parenthesize::IfBreaks) | |
| ] | |
| ) | |
| } else { | |
| write!( | |
| f, | |
| [ | |
| text("assert"), | |
| space(), | |
| test.format().with_options(Parenthesize::IfBreaks) | |
| ] | |
| ) | |
| } | |
| write!(f, [ | |
| text("assert"), | |
| space(), | |
| test.format().with_options(Parenthesize::IfBreaks), | |
| ])?; | |
| if let Some(msg) = msg { | |
| write!( | |
| f, | |
| [ | |
| text(","), | |
| space(), | |
| msg.format().with_options(Parenthesize::IfBreaks) | |
| ] | |
| ) | |
| } | |
| Ok(()) |
There was a problem hiding this comment.
Tbh I don't even see this as a nit lol.
There was a problem hiding this comment.
I ended up adding a group, so I've deviated from your suggestion. I'm still finding my feet with some of this stuff, so feel free to tell me to stop over-implementing this.
I'd want to format this next
# Regression test for https://github.com/psf/black/issues/3414.
assert xxxxxxxxx.xxxxxxxxx.xxxxxxxxx(
xxxxxxxxx
).xxxxxxxxxxxxxxxxxx(), (
"xxx {xxxxxxxxx} xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
)And then finally figure out comment placement, but I can rip out the fixture I've added for that and follow up after this is merged.
There was a problem hiding this comment.
Nice. Yeah, it can sometimes be very subtle.
Yea a couple of things, but one we can probably follow up on if you want to just get this merged. First, the Basically just need long strings causing line-width violation to wrap with parentheses. The second one is one I personally was looking forward to tackling, but it's probably something I could follow up with after this pass is merged. A trailing own-line comment for the # Comment
assert (
# Leading
True # Trailing same-line
# Trailing own-line
), "msg" # Comment
# Commentcurrently becomes # Comment
assert (
# Leading
True # Trailing same-line
), (
# Trailing own-line
"msg"
) # Comment
# Comment |
|
Hmm there's more to this than what I mentioned. I added
|
|
I don't have a good answer for you yet but am looking into a similar problem with My understanding from Black's code is that |
| text(","), | ||
| space(), | ||
| // `msg` gets parentheses if expanded so we don't need any beyond that. | ||
| parenthesize_if_expands(&msg.format().with_options(Parenthesize::Never)) |
There was a problem hiding this comment.
Is the formatting different from msg.format().with_options(Parenthesize::IfBreaks)?
There was a problem hiding this comment.
Yea I'm definitely not satisfied with it yet.
Without my "hack" here I would get
black_compatibility@simple_cases__composition_no_trailing_comma.py.snap
349 │- } == expected, (
350 │- "Not what we expected and the message is too long to fit in one line"
351 │- )
360 │+ } == expected, "Not what we expected and the message is too long to fit in one line"
black will wrap the msg (lines 349-351). So IIUC Parenthesize::IfBreaks wouldn't give me that alone since the msg doesn't break as a long string like that.
It may also be that e.g. this is caused by incorrect formatting of string
I don't know enough yet but maybe this is possible.
There was a problem hiding this comment.
My understanding from Black's code is that assert should work the same as for if headers
This could be helpful. I'll check it out. Thank you!
There was a problem hiding this comment.
Let's see if #5708 helps. It changed the logic that decides when a string needs optional parentheses.
There was a problem hiding this comment.
I spent some more time reading black's source code and what I think we miss is that strings have a higher split priority than any math operator. I'm not sure how we want to model this yet but I suggest to not block this PR because of it.
There was a problem hiding this comment.
Noice. Perfect timing. I just sat down for lunch. Was just about to get back in here.
If you're okay with it, I'd also like to get this merged without the extra comment placement. I think I can follow up on that since I don't think it warrants blocking this either.
|
Did a skim of the updated snapshots. I can check more thoroughly after work, but for now I'll mark this as ready for a review. |
|
Hmm. Scratch that. |
| # TODO: https://github.com/astral-sh/ruff/pull/5168#issuecomment-1630767421 | ||
| # Leading assert | ||
| assert ( | ||
| # Leading test value | ||
| True # Trailing test value same-line | ||
| ), ( | ||
| # Trailing test value own-line | ||
| "Some string" | ||
| ) # Trailing msg same-line | ||
| # Trailing assert | ||
|
|
||
| # Random dangler | ||
|
|
||
| # TODO: https://github.com/astral-sh/ruff/pull/5168#issuecomment-1630767421 | ||
| # Leading assert | ||
| assert ( | ||
| # Leading test value | ||
| True # Trailing test value same-line | ||
| ), ( | ||
| # Trailing test value own-line | ||
| # Test dangler | ||
| "Some string" | ||
| ) # Trailing msg same-line | ||
| # Trailing assert |
There was a problem hiding this comment.
I thought it'd be better to leave this here instead of ripping it out entirely.
|
Got it |
|
|
||
| triple_quote_string = """This is a really really really long triple quote string assignment and it should not be touched.""" | ||
|
|
||
| -assert ( | ||
| - some_type_of_boolean_expression | ||
| -), "Followed by a really really really long string that is used to provide context to the AssertionError exception." | ||
| +NOT_YET_IMPLEMENTED_StmtAssert | ||
| +assert some_type_of_boolean_expression, "Followed by a really really really long string that is used to provide context to the AssertionError exception." |
There was a problem hiding this comment.
Ehh, this is interesting. It seems black happily wraps identifiers in parentheses in some positions but not all. This is fine for now.
There was a problem hiding this comment.
i wonder if that's because assert is somewhat unusual in that one must not wrap the entire rhs in parens, i.e.
assert foo, "msg" very much != assert (foo, "msg")
|
To add some numbers to "increases coverage", the similarity index (percentage of lines we format like black) went from 80.3% to 92.8% for warehouse, the software powering pypi |
Summary
Format
assertstatement withParenthesize::IfBreaksfor test and message expressions.Test Plan
Add ruff fixture with comments.
There's some coverage already as well including:
assert left > rightassert test, msgI'll try to leave the good first issue ones alone now :)
This looks really good btw.
TODO (required):
test's valuemsglong strings UPDATE: commentTODO (extra):