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

Use let else #8358

Closed
wants to merge 2 commits into from
Closed

Use let else #8358

wants to merge 2 commits into from

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Apr 11, 2023

Objective

Now that let...else is stable, we can improve legibility and reduce indentation in code that currently uses if let, followed by large blocks of code.

Solution

Replace instances of if let or match...return to use let...else.

The first commit in this PR fixes instances of clippy::manual_let_else, which detects basic instances of match that we can replace easily. The second commit in this PR additionally changes large function and loop blocks to return or continue early when a value is not matched. This unfortunately causes a lot of changed lines in the PR.

We can just cherry pick the first commit if we don't want to change beyond the pedantic clippy lint, or break these changes up into multiple PRs.

EDIT: For any potential reviewers, you might consider viewing the changes in your local editor or somewhere else that ignores indentation changes when highlighting lines.

@IceSentry
Copy link
Contributor

IceSentry commented Apr 11, 2023

There's already a PR #7104 but it's currently blocked on let else being supported by rustfmt

@B-Reif
Copy link
Contributor Author

B-Reif commented Apr 11, 2023

I probably should have checked for that before working on this.

@IceSentry
Copy link
Contributor

IceSentry commented Apr 11, 2023

With that said, your PR is much more recent, so if it gets support soon at least your PR is more up to date and therefore more likely to get merged

@IceSentry
Copy link
Contributor

For the tracking issue in the rustfmt side rust-lang/rustfmt#4914

@B-Reif
Copy link
Contributor Author

B-Reif commented Apr 11, 2023

It looks like rustfmt's implementation work is more-or-less there also. Apparently held up by 'release/communication work'.
rust-lang/rustfmt#5690

@harudagondi harudagondi added C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes labels Apr 12, 2023
@B-Reif
Copy link
Contributor Author

B-Reif commented Apr 14, 2023

let...else has been blocked on rustfmt for nearly four months now. Would we consider using a fork of rustfmt to get this unblocked until they can get this thing merged? This release gap is starting to seem unreasonable.

@mockersf
Copy link
Member

EDIT: For any potential reviewers, you might consider viewing the changes in your local editor or somewhere else that ignores indentation changes when highlighting lines.

You can do that in GitHub UI for reviews:

Screenshot 2023-04-14 at 19 14 19

Would we consider using a fork of rustfmt to get this unblocked until they can get this thing merged?

I don't think let ... else is an improvement big enough for that... so no for me

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Sep 20, 2023
@alice-i-cecile
Copy link
Member

This is no longer blocked, but it's going to be easier to just remake this than to fix the merge conflicts :) Please feel free to do so: this is generally a really nice change to make!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants