-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
There was a problem hiding this 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.
Think that's a fair take. I'll revert when I get a chance. |
There was a problem hiding this 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.
|
||
// 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.
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.
@@ -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}"))?, |
There was a problem hiding this comment.
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.
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
650 Running 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/