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

Apply fixes from -Wclippy::pedantic #13

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

chris-ha458
Copy link
Contributor

I just used cargo clippy -- -Wclippy::pedantic to take a look into some potential fixes.

Some are straightforward and obvious, but some are more opinionated.
Let me know what you think!

Copy link
Owner

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

Hello - Most of the changes here are reasonable, although there is a reason clippy calls them pedantic and I wouldn't make that the default lint setting in this repository. The one change I wouldn't apply is the for-loop => fold (which incidentally, shows that a test is missing).

@@ -64,7 +64,7 @@ where
queue.push(Partition {
score: score(boundary),
boundary,
obstacles: obstacles.to_vec(),
obstacles: obstacles.clone(),
Copy link
Owner

Choose a reason for hiding this comment

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

I slightly prefer to_vec here because it makes the type more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into the code, obstacles is already made into a vec above
line 57 let mut obstacles = obstacles.to_vec();
Since is not consumed elsewhere later I think we can just use the shorthand initialization without .clone() or to_vec()

it would look like this

queue.push(Partition {
                score: score(boundary),
                boundary,
                obstacles,
            });

do you still prefer the original over this?

Copy link
Owner

Choose a reason for hiding this comment

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

You're right. The clone looks redundant. This could be made clearer by moving initialization of it inside the !boundary.is_empty() check.

ocrs/src/preprocess.rs Outdated Show resolved Hide resolved
decode_method: self.decode_method,
},
)
if let Some(recognizer) = self.recognizer.as_ref() {
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK, although if new lines were added to the happy path, I'd probably want to revert back to the early-exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I understand correctly, the two versions of the code operate identically and the
if let Some vs let - else construction merely moves the main body vs exception placements around. This arguably makes it clearer what this part of the code intends to do vs what it does when it fails.

Usually let else is used when there are a lot of following code, where the if let Some pattern will introduce braces and tab drift.
Of course, if we assume both operate the same it becomes a matter of choice, and the opinion of the main maintainer/developer matters more IMO

If i inadvertently introduced any change in behavior I think this should also revert back to the original.

Copy link
Owner

Choose a reason for hiding this comment

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

Usually let else is used when there are a lot of following code, where the if let Some pattern will introduce braces and tab drift.

Yes, this is one reason. The other is to logically have early-exit checks at the top of the function and then have the main body concentrate on the happy path. This also matters mainly when there are multiple early-exits. With the current body both are easy to read, and I would be fine with either.

@robertknight robertknight changed the title small fixes Apply fixes from -Wclippy::pedantic Jan 7, 2024
Copy link
Owner

@robertknight robertknight 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 the contribution.

decode_method: self.decode_method,
},
)
if let Some(recognizer) = self.recognizer.as_ref() {
Copy link
Owner

Choose a reason for hiding this comment

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

Usually let else is used when there are a lot of following code, where the if let Some pattern will introduce braces and tab drift.

Yes, this is one reason. The other is to logically have early-exit checks at the top of the function and then have the main body concentrate on the happy path. This also matters mainly when there are multiple early-exits. With the current body both are easy to read, and I would be fine with either.

@robertknight robertknight merged commit 3560bf3 into robertknight:main Jan 8, 2024
2 checks passed
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.

2 participants