-
Notifications
You must be signed in to change notification settings - Fork 917
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
Add match_arm_indent
option
#6525
Conversation
match_arm_indent
optionmatch_arm_indent
option
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.
Thanks for taking the time to work on this older feature request.
The code seems pretty straightforward to me, but I've left some comments inline. Take a look when you've got a chance.
a018179
to
1b27957
Compare
Applied the changes, moved the tests to the |
Thanks for making the changes and expanding on the test case. I'll have some time to re-review this early next week. |
fn silly() { | ||
match value { | ||
Inner(i1) => match i1 { | ||
Inner(i2) => match i2 { | ||
Inner(i3) => match i3 { | ||
Inner(i4) => match i4 { | ||
Inner => "it's a readability tradeoff, really", | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
} |
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.
Good test case, but it's pretty unfortunate formatting. It's really hard to tell that these are nested match statements.
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.
Yeah, that's the main problem with this formatting option. The example is deliberately obtuse, but I think it's somewhat better with realistic usage. That's how I envision it working for AST matching, for example:
match statement {
Stmt::Local(item) => match item {
Item::Const(item_const) => {
// do stuff with const
},
Item::Enum(item_enum) => {
// do stuff with enum
},
// rets of items
},
Stmt::Expr(expr, semi) => match expr {
Expr::Array(arr) => {
// do stuff with arr (and semi, with is in scope despite not being in the second match statement)
},
Expr::Assign(assign) => {
// ...
},
// ...
},
}
Here the flat structure is desirable, as we're processing all of the items together.
In theory, there could be a heuristic which would wrap the nested match as a body, indenting it. But I'm not sure how it could be applied so that it both fixes these edge cases, but doesn't break sane nesting
I went ahead and created a tracking issue for this #6533. When you get a chance please rebase the PR and add the tracking issue link to the option's documentation. |
1b27957
to
2569046
Compare
Added the link to the tracking issue |
Allows to disable the indentation of match arms. Related issue: rust-lang#2937 Co-authored-by: Yacin Tmimi <[email protected]>
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.
Overall the implementation is pretty straightforward and doesn't complicate match arm formatting. I still think the nested cases are hard to read, but I'd say we're fine to move forward with this as an unstable option.
Thanks for taking the time to review it! |
Just an FYI the new option won't be available on nightly until the next subtree sync with rust-lang/rust. hoping to get that out soonish |
Oh, yeah, I'm not in a hurry. I know that things take time when it comes to releases. I'm actually on stable myself, so I was hoping that someone on nightly would pick up this feature and test it in the meanwhile |
Allows to disable the indentation of match arms.
Related issue: #2937