Skip to content
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

Merged
merged 1 commit into from
Apr 3, 2025
Merged

Conversation

KAAtheWiseGit
Copy link
Contributor

Allows to disable the indentation of match arms.

Related issue: #2937

@KAAtheWiseGit KAAtheWiseGit changed the title feat: add match_arm_indent option Add match_arm_indent option Mar 29, 2025
Copy link
Contributor

@ytmimi ytmimi left a 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.

@KAAtheWiseGit KAAtheWiseGit marked this pull request as draft March 30, 2025 09:07
@KAAtheWiseGit KAAtheWiseGit force-pushed the match-arm-indent branch 3 times, most recently from a018179 to 1b27957 Compare March 30, 2025 09:56
@KAAtheWiseGit
Copy link
Contributor Author

Applied the changes, moved the tests to the configs/match_arm_indent folder, and added a bunch more tests. I also remembered that this feature interacts with nested matches, so I added a few cases for that

@KAAtheWiseGit KAAtheWiseGit marked this pull request as ready for review March 30, 2025 09:59
@ytmimi
Copy link
Contributor

ytmimi commented Mar 30, 2025

Applied the changes, moved the tests to the configs/match_arm_indent folder, and added a bunch more tests. I also remembered that this feature interacts with nested matches, so I added a few cases for that

Thanks for making the changes and expanding on the test case. I'll have some time to re-review this early next week.

Comment on lines +67 to +79
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",
},
},
},
},
}
}
Copy link
Contributor

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.

Copy link
Contributor Author

@KAAtheWiseGit KAAtheWiseGit Apr 2, 2025

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

@ytmimi ytmimi mentioned this pull request Apr 2, 2025
4 tasks
@ytmimi
Copy link
Contributor

ytmimi commented Apr 2, 2025

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.

@KAAtheWiseGit
Copy link
Contributor Author

KAAtheWiseGit commented Apr 2, 2025

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]>
@ytmimi ytmimi force-pushed the match-arm-indent branch from 2569046 to 9a90a7a Compare April 3, 2025 01:28
Copy link
Contributor

@ytmimi ytmimi left a 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.

@ytmimi ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Apr 3, 2025
@ytmimi ytmimi merged commit 5688caa into rust-lang:master Apr 3, 2025
26 checks passed
@KAAtheWiseGit
Copy link
Contributor Author

Thanks for taking the time to review it!

@ytmimi
Copy link
Contributor

ytmimi commented Apr 3, 2025

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

@KAAtheWiseGit
Copy link
Contributor Author

KAAtheWiseGit commented Apr 3, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants