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

refactor: prefer 'if let' to 'match' for single option #28347

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

danieleades
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2025

CLA assistant check
All committers have signed the CLA.

@danieleades danieleades marked this pull request as draft February 28, 2025 16:00
@danieleades danieleades marked this pull request as ready for review February 28, 2025 16:25
@danieleades danieleades changed the title style: prefer 'if let' to 'match' for single option refactor: prefer 'if let' to 'match' for single option Feb 28, 2025
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Sorry, but this creates some churn in the codebase and I don't believe it matters? Feels better to leave it up to the person writing the code whether they'd prefer to use match or if. I also prefer the match in some of the cases. Maybe if there was a lot of indentation, but that doesn't seem to be a problem in most of these cases.

Overall, we'd rather not do style PRs like this on the codebase.

@danieleades
Copy link
Author

Overall, we'd rather not do style PRs like this on the codebase.

That's an interesting perspective. If that's the position I'll of course respect that.

Yes, the main benefit is reducing indentation. I actually think this is a problem in this code base, somewhat masked by the unusual (in rust) 2-space indentation.

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.

3 participants