Ensure space around binary exprs#6085
Conversation
tests/target/issue_6059.rs
Outdated
| @@ -0,0 +1,3 @@ | |||
| fn float_range() { | |||
| let _range = 3. / 2. ..4.; | |||
There was a problem hiding this comment.
Thanks for your help on this!
What happens if the 2. was written as 2.0? Do we still add the space? Can you include a test case for that.
There was a problem hiding this comment.
No worries! If the literal ends with a dot it doesn't add a space, I think that's intended right? I added a test case for it
src/expr.rs
Outdated
| ast::ExprKind::Binary(_, ref expr, _) => { | ||
| needs_space_before_range(context, expr) | ||
| } |
There was a problem hiding this comment.
Is this correct? The ast::ExprKind::Binary is defined as Binary(BinOp, P<Expr>, P<Expr>), and I'm assuming that the first expr is the left expr (the one that comes before the operator), while the second is the right expr (the one that comes after the operator). The way the code is written it looks like you're testing whether the left expr needs a space before the range, when I'd expect this to check if the right expr needs a space before the range.
There was a problem hiding this comment.
It's incorrect, you're absolutely right, nice catch!
Changed to the rhs and added the source example and the other test cases.
| @@ -0,0 +1,3 @@ | |||
| fn float_range_no_trailing_dot_compacts() { | |||
| let _range = 3.0 / 2.0..4.0; | |||
There was a problem hiding this comment.
Lets add the following test case:
let _range = 3. / 2.0..4.0;| @@ -0,0 +1,3 @@ | |||
| fn float_range_trailing_dot() { | |||
| let _range = 3. / 2. ..4.; | |||
There was a problem hiding this comment.
Let's add the following test case:
let _range = 3.0 / 2. ..4.;
ytmimi
left a comment
There was a problem hiding this comment.
The code changes seem pretty straightforward. Just want to make sure that we're testing the correct expression when adding a check for ast::ExprKind::Binary in needs_space_before_range. I know needs_space_before_range takes the lhs of the range, but we need to make sure we're testing the rhs of the binary expression.
I've requested some additional test cases to help confirm that.
|
Also I think it would be helpful if we could work the original code snippet from #6059 into a test case. |
abb10bd to
d5674a3
Compare
ytmimi
left a comment
There was a problem hiding this comment.
Just took another look at the PR, and now that we're passing the rhs_expr to needs_space_before_range it looks like rustfmt is producing the correct formatting. Thanks again for the help on this one!
Fixes #6059.
It just needed the same caveat for binary exprs.
It was already implemented for unaries so
2. .. 4.worked but not4. / 3. .. 5.for example.