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] Removing most of clippy warnings #88

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

JulesGuesnon
Copy link
Contributor

@JulesGuesnon JulesGuesnon commented Nov 21, 2023

Description

This PR applies most of the suggestions made by clippy.
There's still some warnings that I didn't take care of:

Also, I think this PR could be a good timing to choose a lint level and explicitly add it everywhere

@kaleidawave
Copy link
Owner

kaleidawave commented Nov 21, 2023

Cool. Changes look good so far!

With the Result<..., ()> those are functions where I haven't quite finished what the error should be. Cool to know they are identified by clippy.

In terms of the too many function parameters. I was considering bundling ThisValue, arguments and other argument kind of things into a struct. That would squash a few parameters into one to fix the lint. If you want to attempt then that would be great 👍

Also slightly more ambitious but I am also thinking about putting Publicity into ProperyKey::String. (You can't have non statically known private keys so should be fine to not be on PropertyKey::Type). That should solve the property key too many parameters.

Don't have to and could merge now if you want but you definitely seem capable :)

In terms of lint levels, do you have any suggestions. I think there is a recently added way to specify them in Cargo.toml. Did you encounter a common rule that you think or should be changed from default? Any other changes to the current default config? I haven't too much experience with clippy to know :)

@kaleidawave
Copy link
Owner

Going to merge this now as I think it is important is added asap. The other points can be addressed in the future with other PRs

@kaleidawave kaleidawave merged commit 5021262 into kaleidawave:main Nov 24, 2023
6 of 8 checks passed
@JulesGuesnon
Copy link
Contributor Author

JulesGuesnon commented Nov 24, 2023

Hey @kaleidawave ! Sorry for the delay, I've been quite busy last days so I couldn't have a look to your feedbacks. I'll be busy until the end of the week I think, but I'll open another PR to address everything you mentioned when I can

@kaleidawave
Copy link
Owner

No worries, I thought so 👍. Wanted to work on the project today so was keen to have the improvements in today to not end up with a bunch of merge conflict.

@kaleidawave kaleidawave added the internal-improvements Related to internal publishing, building, workflows, formatting & linting label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-improvements Related to internal publishing, building, workflows, formatting & linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants