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: Apply Clippy lint comparison_chain #5771

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

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Jan 30, 2025

Description

Apply Clippy lint comparison_chain, which finds places where if ... else if statements can be replaced with match a.cmp(b)

There were a few locations where I didn't want to make this change, because I felt it would decrease readability by increasing the indent level for a large segment of code. I added #[allow(clippy::comparison_chain)] so Clippy will ignore them

@jbencin jbencin added the lint Related to linting/clippy/cargo warns label Jan 30, 2025
@jbencin jbencin requested a review from a team as a code owner January 30, 2025 01:13
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This lint feels strictly worse than what it replaces to me. If others feel differently, that’s okay with me, but for my part, it just seems to decrease readability by a good amount (not only because I have to remember the directionality of cmp, but because I have to look back to the match opening to even see what’s being compared)

@jbencin
Copy link
Contributor Author

jbencin commented Jan 30, 2025

@kantai I had mixed feelings on this, and some of the suggestions I skipped for this reason. Some of these do still result in more lines/indentation, but I think doing the cmp once and then matching the result is more clear than repeatedly comparing the same two things in an if ... else if ... else statement

I have to remember the directionality of cmp

It's intuitive, it's left-to-right just like you would read it. For example, 2.cmp(&4) returns Ordering::Less

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint Related to linting/clippy/cargo warns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants