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

[lint] Fix some clippy warnings on Cargo manifests #616

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

woshilapin
Copy link
Contributor

clippy::cargo is not enabled by default but can provide some useful hints on Cargo.toml manifests. However, there is no easy way to solve the problem of multiple versions of dependencies so I specifically deactivated this one.

@ArnaudOggy ArnaudOggy requested a review from datanel April 24, 2020 15:36
Copy link
Member

@datanel datanel left a comment

Choose a reason for hiding this comment

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

cargo:clippy includes

  • cargo_common_metadata (what you changed in this PR)
  • multiple_crate_versions (excluded in your PR)
  • wildcard_dependencies (I think we will never include "*" as version)

Now that you applied all the recommandations from cargo_common_metadata, is it useful to always check clippy::cargo ?
May be if we add a new "sub-crate". I don't see well the benefice.

@woshilapin
Copy link
Contributor Author

Now that you applied all the recommandations from cargo_common_metadata, is it useful to always check clippy::cargo ?

I would say yes, for the same reasons that we didn't remove clippy in general even if we already fixed all the warnings and errors (I agree changes on Cargo.toml are much less frequent but still).

May be if we add a new "sub-crate". I don't see well the benefice.

A few reasons to keep it, in my opinion:

  • new developers might come in, not necessarily aware of all our practices
  • new sub-crates might be created
  • new clippy::cargo rules might appears (today, only 3 but more might be created)

@datanel datanel merged commit 32bdd04 into hove-io:master Apr 27, 2020
@woshilapin woshilapin deleted the cargo-clippy branch June 29, 2020 11:25
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.

3 participants