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

Clippy #102

Merged
merged 10 commits into from
Jan 31, 2025
Merged

Clippy #102

merged 10 commits into from
Jan 31, 2025

Conversation

srhb
Copy link
Contributor

@srhb srhb commented Jan 23, 2025

A few clippy hints remain that I don't know how to deal with, need a hand

As I understand it, we get warns for dead code because we're _only_
using the fields here via their Debug-instances (eg. for logs)

Since we're actually just-fine-thank-you-very-much with the
autogenerated strings, this seems needlessly strict to us and creating
custom Display-instances just to squelch this seems even noisier than
allowing the dead_code fields.
@srhb
Copy link
Contributor Author

srhb commented Jan 29, 2025

@srhb srhb marked this pull request as ready for review January 29, 2025 15:50
@srhb
Copy link
Contributor Author

srhb commented Jan 29, 2025

Need help with the remaining failures (run cargo clippy from the devShell)

@cafkafk cafkafk self-requested a review January 30, 2025 05:23
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Some are a bit nit picky, and shouldn't be considered blocking

@cafkafk
Copy link
Member

cafkafk commented Jan 30, 2025

I wasn't sure how to best show solving the last clippy lints without having something to point at so I made a commit 0f9367c and added some GitHub comments. I hope that's somewhat useful. It did get hit with a rust fmt which makes the diff unnecessarily large, but I've pointed at the parts of the code that were changed, and tried to show why clippy didn't give great hints (clippy didn't fully know to explain what the changes meant for the impl's using the trait)

@cafkafk cafkafk mentioned this pull request Jan 30, 2025
4 tasks
@srhb
Copy link
Contributor Author

srhb commented Jan 30, 2025

Thanks for the help, think I've incorporated it all (and dropped the formatting for now so we don't mix that up in this already-large diff)

@srhb srhb requested a review from cafkafk January 30, 2025 11:19
@cafkafk cafkafk merged commit c73c910 into main Jan 31, 2025
1 check passed
@cafkafk cafkafk deleted the clippy branch January 31, 2025 07:09
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