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 most clippy pedantic and nursery lint warnings #124

Closed
wants to merge 30 commits into from

Conversation

willbush
Copy link
Member

@willbush willbush commented Oct 30, 2024

After seeing #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 willbush changed the title Enable clippy pedantic lint warning Enable clippy pedantic and nursery lint warnings Oct 30, 2024
Copy link
Contributor

@Ben-PH Ben-PH left a comment

Choose a reason for hiding this comment

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

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.

src/main.rs Outdated Show resolved Hide resolved
src/ratchet.rs Outdated Show resolved Hide resolved
src/references.rs Outdated Show resolved Hide resolved
@willbush
Copy link
Member Author

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 willbush requested a review from Ben-PH October 30, 2024 17:40
Copy link
Contributor

@Ben-PH Ben-PH left a comment

Choose a reason for hiding this comment

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

Generally speaking I find that pedantic lints should be thought of as "opinionated nits". I like to turn them on, give it a quick pass-through, and turn them off again. I'll leave behind the things that I used to filter out the noise.

In this PR the main thing that is of substantive value is the cleaning up of arguments with respect to references, as well as passing in a ref to Path instead of PathBuf (thus removing the pathalogical constraint of "path must be on the heap and owned"). Everything else, for the most part, puts a nice polish. Over time, the cumulative gains of PRs such as this, I can see being really valuable though.

Comment on lines +535 to +537

// Allow pedantic lint: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
// fixing it hurts readability of code due to comment restructuring
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not needed. The process of understanding this area of the code base would now also need the reader to filter out these comments, detracting from readability.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/nix_file.rs Outdated Show resolved Hide resolved
src/problem/npv_163.rs Show resolved Hide resolved
@@ -85,7 +83,7 @@ fn check_path(
)
})
.collect_vec()
.with_context(|| format!("Error while recursing into {}", subpath))?,
.with_context(|| format!("Error while recursing into {subpath}"))?,
Copy link
Contributor

Choose a reason for hiding this comment

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

personally, I prefer non-inlined. Probably out of inertia/habit more than anything. The only substantive comment I have is that you can't do {struct.field}, meaning that sometimes you inline, sometimes you don't, bringing some inconsistencies.

Even so, I see spending too much time on this one way or the other as bike-shedding, so in any case, go ahead, and if it becomes an actual issue, we can revisit.

@willbush
Copy link
Member Author

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:

  • might catch potential issues early
  • might lower mentioning nits in PR

cons:

  • might require frequent suppression
  • suppression is distracting
  • might be a form of unnecessary friction

@willbush
Copy link
Member Author

willbush commented Nov 8, 2024

cons:

* might require frequent suppression

* suppression is distracting

* might be a form of unnecessary friction

Think I talked myself out of this. Just going to mention it in contributing.md as something one might want to checkout.

@willbush willbush changed the title Enable clippy pedantic and nursery lint warnings Fix most clippy pedantic and nursery lint warnings Nov 8, 2024
@willbush willbush requested a review from Ben-PH November 8, 2024 11:03
@willbush
Copy link
Member Author

willbush commented Nov 8, 2024

Just going to close and perhaps recreate in another PR to cleanup the commits

@willbush willbush closed this Nov 8, 2024
@willbush willbush mentioned this pull request Nov 8, 2024
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