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

chore: use let-else anywhere possible and practical #10742

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented May 12, 2024

Drive-by MR to shorten lots of simples matches, removing code to read is nice

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of refactor is good when you're already rewriting a block in a PR but converting everything will just make a bunch of conflicts for open PRs - only for a cosmetic benefit

@poliorcetics
Copy link
Contributor Author

It's only a few lines where looking at the reason for the diff makes it a very very easy conflict to fix, I think the refacto is worth it. What's more, I don't think the parts I touched are modified all that often, else they would probably have been caught in the 13 versions of Rust since let-else was stabilized

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this change, it mostly changes small blocks that were a standalone expression (something or early return/error)

@the-mikedavis the-mikedavis requested review from the-mikedavis and removed request for the-mikedavis May 15, 2024 01:45
@the-mikedavis
Copy link
Member

The conflicts are easy, it's just making work for anyone with a branch that touches any of this code and for no practical benefit: it's purely cosmetic. I don't feel super strongly about this so I've removed my red 'X' and anyone can free to merge this but my opinion is that I'd rather see this closed than merged.

@the-mikedavis the-mikedavis removed their request for review May 15, 2024 01:47
@pascalkuthe
Copy link
Member

I would be ok with some touchups to the DAP code since we genuinely never touch that. For moster other code change here there are open PRs (or stuff in the works that doesn't yet have a PR) that touches that. I think I agree with Mike here that touchupsnlike this aren't worth it when they cause conflicts.

They may seem minor but they do definitly add up a lot. I already spent a lot of time conflict resolving. Even if most conflicts are minor just going trough all of them takes time. Especially because you need to read conflicts very carefully to missing subtle changes. For example after this PR somebody else may fix a off by one error. If I would just scan a conflict and just see "this is a let ... else change" and then change the new code to also let else I wouldnlikely miss the mor subtle fix (something like this has happened to me before but I thankfully caught it myself since it caused a noticeable regression but thay isn't always the case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants