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

fix some lints #1421

Merged
merged 16 commits into from
Dec 6, 2022
Merged

fix some lints #1421

merged 16 commits into from
Dec 6, 2022

Conversation

danieleades
Copy link
Contributor

fixes a bunch of clippy lints. There's a couple of potentially controversial commits in there, particularly the elimination of unused 'self' arguments

hannobraun
hannobraun previously approved these changes Dec 6, 2022
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request, @danieleades!

I like all these changes, with the exception of the Clippy MSRV. I've left a comment there. I'm going to drop that commit and merge this pull request immediately, as it isn't going to age well. If left unmerged, it would have conflicts very soon.

If you disagree with my comment on the MSRV, feel free to submit that again in a new pull request. I don't want to kill any discussion, I just want to get the rest of this merged as soon as possible.


Since the code already builds without Clippy warnings (as verified by the CI build), I assume you used a non-default Clippy configuration. Would you mind submitting that configuration too, so the code doesn't regress?

.clippy.toml Outdated
@@ -0,0 +1 @@
msrv = "1.65.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't that redundant with rust-toolchain.toml, since that already defines which Clippy version we're running with? I'd rather not have another place which I need to remember to update, unless there is a clear benefit.

Copy link
Contributor Author

@danieleades danieleades Dec 6, 2022

Choose a reason for hiding this comment

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

no, clippy doesn't parse the rust-toolchain.toml file. The benefit is that you can run the nightly clippy, but avoid any clippy suggestions that would violate the msrv.

For example, inline format args were added in 1.58, but the clippy suggestion to update the syntax didn't arrive until 1.65. Specifying the clippy MSRV in the config file allows you to use the nightly clippy and get the up-to-date suggestions right away

Copy link
Owner

Choose a reason for hiding this comment

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

I didn't mean to say that Clippy parses the rust-toolchain.toml. What I meant was that the contents of the rust-toolchain.toml define what version of Clippy is going to run.

But I understand why we need this now. You can still run cargo +nightly clippy to override the specified toolchain, and then this configuration would be useful.

Thanks for explaining!

@hannobraun hannobraun enabled auto-merge December 6, 2022 09:09
@hannobraun hannobraun merged commit 628c1e2 into hannobraun:main Dec 6, 2022
@danieleades danieleades deleted the clippy branch December 6, 2022 10:12
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