pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

Enable some Clippy lints

Open nickdrozd opened this issue 10 months ago • 5 comments

Hello! This PR enables some Clippy lints and fixes all the warnings. There is one lint enabled per commit (except for the pyo3-macros) one, so I would suggest reviewing the commits one at a time. The changes all seem reasonable, and hopefully not too intrusive. It doesn't do pyo3-macros-backend, but I can add that too if that's wanted.

nickdrozd avatar Apr 25 '24 00:04 nickdrozd

I think it might be better to open a PR for each lint. That would make it easier to say "yes we want this" or "no we don't want this" on a case-by-case basis.

Finally, for this to land, we'll need to fix the lints for all CI builds.

I dropped the const and semicolon lints, since those changed a lot of lines and were both failing in CI (hard to check locally because of all the various config flags). I can still break up the remaining commits if you like, but I think it should be a lot more manageable now.

nickdrozd avatar Apr 25 '24 14:04 nickdrozd

I didn't realize there was such deviation between a local run and a CI run :face_with_spiral_eyes:

nickdrozd avatar Apr 25 '24 14:04 nickdrozd

I didn't realize there was such deviation between a local run and a CI run 😵‍💫

You can reproduce most of that locally using the Nox sessions which e.g. enumerate feature sets.

adamreichold avatar Apr 25 '24 15:04 adamreichold

For example, the missing_const_for_fn lint is explicitly in development and has a bunch of caveats.

Especially since what could be const today may not be able to be const tomorrow. IMO it's a bit irresponsible of Clippy to integrate such lints that encourage API breaking or major-version inflation, seeing as how people tend to apply its suggestions without much critical thinking (which, in many cases, is justified).

Edit: I see it's allow-by-default. That makes more sense, and once it's out of the nursery it will probably go to a lint group that we definitely don't want to have in a sweeping #[deny].

birkenfeld avatar Apr 26 '24 18:04 birkenfeld

I'm still not convinced we necessarily want all of these, so I'd prefer to have them as separate PRs.

LilyFoote avatar May 01 '24 16:05 LilyFoote