search

NixOS / nixpkgs-vet

Tool to vet (check) Nixpkgs, including its pkgs/by-name directory
MIT License
29 stars 7 forks source link

Enable clippy pedantic and nursery lint warnings #124

Open willbush opened 5 days ago

willbush commented 5 days ago

After seeing https://github.com/NixOS/nixpkgs-vet/pull/120 I was looking at other pedantic lints and realized.. I must be pedantic because I like most of the suggestions lol. I think because this project is small having this on doesn't seem too painful. Let me know what you think!

edit: note different lint categories https://doc.rust-lang.org/nightly/clippy/

willbush commented 5 days ago

In my opinion, the changes that I haven't commented on bring a net benefit. At worst, they make the code base no-better/no-worse, but that would require an inappropriately pessimistic view.

I do, however, strongly disagree with linting for module name repetition. With the file store for example, the changes made project the wrong intent. It is not some general store. Allowing it to be used as just a Store, projects the wrong notion. For this item to clearly communicate what it does, it needs to only be usable with the full name.

When I first discovered pedantic, I tried playing around with approaches to satisfy this lint in a way that makes sense. I came to the conclusion that this particular lint almost always raises false-positives.

Think that's a fair take. I'll revert when I get a chance.

willbush commented 4 days ago

I like to turn them on, give it a quick pass-through, and turn them off again.

I'm not opposed to pedantic being off by default. Perhaps we could just mention it in the CONTRIBUTING.md as something to consider looking at. I guess I'm conflicted on one point though:

$ cargo clippy --all-targets -- -W clippy::nursery -W clippy::pedantic 2>&1 | wc -l
650

Running it (on main) produces a lot of output and it would build up overtime. The output would need to basically need to be diffed before / after PR changes which I supposed we could automate with a script. And perhaps even a GH action to show the new lints in the PR.

pros:

cons: