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: fix some Clippy lints #1068

Merged
merged 13 commits into from
Oct 25, 2024
Merged

chore: fix some Clippy lints #1068

merged 13 commits into from
Oct 25, 2024

Conversation

allan2
Copy link
Contributor

@allan2 allan2 commented Oct 23, 2024

No description provided.

@allan2
Copy link
Contributor Author

allan2 commented Oct 25, 2024

Ready for review. This PR fixes easy and non-ambiguous lints.

Remaining unfixed lints include:

  • unused_crate_dependencies
  • unreachable_pub - most warnings are in Agent; I assume more modules will be exported in the future
  • unused_qualifications - I did not replace std::mem::size_of/mem::size_of with size_of
  • clippy::as_underscore

More PRs to follow for the remaining ones.

@CBenoit
Copy link
Member

CBenoit commented Oct 25, 2024

Thank you!

* unused_crate_dependencies

If you want to address this one, let me know beforehand so we can discuss on how it should be approached.

* unreachable_pub - most warnings are in Agent; I assume more modules will be exported in the future

Actually, it’s probably a sign that the pub keyword should be removed, or pub(crate) should be used.
In some cases such a sealed traits, we actually want to silent this warning though.

* unused_qualifications - I did not replace `std::mem::size_of`/`mem::size_of` with `size_of`

For that one, in order of preference, I recommend: mem::size_of > std::mem::size_of > size_of

  • mem::size_of is the best readability and clarity overall
  • std::mem::size_of when the function is used only once
  • size_of when it is used a lot for doing arithmetic, I would then recommend a local import at the function level where it is used abundantly

That being said, those are just guidelines, and anything is acceptable :)

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

I removed the duplicated commits from master:

tmpscrot

Maybe something wrong happened when you tried to rebase on top of master or merge master into your branch?

Otherwise, everything is looking good to me!

@CBenoit
Copy link
Member

CBenoit commented Oct 25, 2024

I reworded a bit your individual commits so we can merge rebase them without squashing everything. It mostly comes down to the fact that changing code without modifying the semantic is a "refactor", changes to doc only (including comments) should be "docs" and I added the scope when it made sense. It’s mostly relevant for features and bug fixes that are actually perceived by the end user as we have scripts looking for these commits when generating the changelog to avoid forgetting important changes. This is based on the conventional commit specification. All that said, I should document the details and how we are using it in the repository directly :)

@CBenoit CBenoit merged commit 0af2779 into Devolutions:master Oct 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants