Fix most clippy pedantic and nursery lint warnings#124
Fix most clippy pedantic and nursery lint warnings#124
Conversation
Ben-PH
left a comment
There was a problem hiding this comment.
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. |
Ben-PH
left a comment
There was a problem hiding this comment.
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.
|
|
||
| // 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 |
There was a problem hiding this comment.
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.
| }) | ||
| .collect_vec() | ||
| .with_context(|| format!("Error while recursing into {}", subpath))?, | ||
| .with_context(|| format!("Error while recursing into {subpath}"))?, |
There was a problem hiding this comment.
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.
I'm not opposed to pedantic being off by default. Perhaps we could just mention it in the $ cargo clippy --all-targets -- -W clippy::nursery -W clippy::pedantic 2>&1 | wc -l
650Running it (on pros:
cons:
|
Think I talked myself out of this. Just going to mention it in contributing.md as something one might want to checkout. |
part of clippy::module_name_repetitions
part of clippy::module_name_repetitions
44e6dc0 to
1628d88
Compare
|
Just going to close and perhaps recreate in another PR to cleanup the commits |
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/