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

style: apply clippy lints #23

Merged
merged 8 commits into from
Aug 2, 2023
Merged

style: apply clippy lints #23

merged 8 commits into from
Aug 2, 2023

Conversation

aidandenlinger
Copy link
Contributor

This PR applies several suggestions from clippy, the built-in Rust linter designed to "catch common mistakes and improve your Rust code". Running cargo clippy on main currently produces 66 warnings, after this PR there are 0. Most of these suggestions are about style, so please feel free to only accept some or none of this PR if they don't align with your coding style! I can remove any commits you don't want to merge, just let me know.

Each lint is in its own commit. You can search up each of the lints on the clippy lints website to see why clippy suggests them.

This compiles and I did a basic profile download without any issues, but I didn't test more thoroughly than that.

If you want to enable clippy hints in your dev environment, in VS Code, you can go to settings and set rust-analyzer.check.command to clippy, for helix you can follow these instructions, otherwise check the internet :)

@CMahaff
Copy link
Owner

CMahaff commented Aug 1, 2023

Thanks for this! I'm new to rust so this taught me some good behaviors I should be using in my rust code going forward.

I think the only lint item I don't really want is needless_return - I think it's better to explicitly indicate when you are returning, at least for normal functions - only really makes sense to omit for closures, etc. IMO.

The rest I'd be happy to take. I can do some testing later - I've got a handful of accounts for that purpose.
It also looks like it's possible to update main.rs to include an entry that suppresses a specific clippy warning for just the "needless_return" type so the number goes to zero. If this is required above every use of return obviously don't bother haha.

I will definitely be turning this on in VS Code going forward - again, thanks, I didn't even know it existed. I wonder if I can even set a rule that all clippy warnings must be resolved before successful PR.... though maybe that's overkill.

@aidandenlinger
Copy link
Contributor Author

aidandenlinger commented Aug 2, 2023

It also looks like it's possible to update main.rs to include an entry that suppresses a specific clippy warning for just the "needless_return" type so the number goes to zero.

Done in latest commit, and I deleted the commit that removed the returns :) So cargo clippy disregards needless_returns and has 0 warnings! Feel free to comment on anything else that needs changing before merging and let me know if testing shows any broken behavior.

Yeah, I wish the IDEs defaulted to using clippy, I'm pretty new to Rust too and stumbling into clippy has been invaluable in learning new ways to do things.

I wonder if I can even set a rule that all clippy warnings must be resolved before successful PR.... though maybe that's overkill.

The clippy documentation has an example Github Actions file, and there's some actions like actions-rs/clippy-check which seem to be nice :) Haven't tried them myself, but doesn't seem too bad if you wanted to enforce it.

And I totally forgot to say this in my initial message, but thank you for writing LASIM! It's been extremely useful in backing up my account and migrating between instances, very grateful for it and happy to contribute :)

@CMahaff CMahaff merged commit 5b0f25f into CMahaff:main Aug 2, 2023
3 checks passed
@aidandenlinger aidandenlinger deleted the clippy branch August 8, 2023 21:59
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

2 participants