-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 Clippy lints #1832
Fix Clippy lints #1832
Conversation
src/utils/fs.rs
Outdated
} else { | ||
fs::remove_file(item)?; | ||
} | ||
for item in fs::read_dir(dir)?.flatten() { |
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.
I find this harder to read, can it be reverted?
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.
Clippy says otherwise, fwiw.
I personally prefer flatten()
due to removing one level of indentation and a couple of lines.
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.
I don't agree with quite a few clippy lints. If it were about lines or indentation, then several of these other changes should not be included because they add more lines and indentation. In this case, it is not as clear what is being flattened as the types are obscured. By checking for Ok(…)
, it is clear that it is explicitly ignoring errors.
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.
Fair point, I'll revert it then.
src/book/book.rs
Outdated
@@ -77,10 +77,10 @@ fn create_missing(src_dir: &Path, summary: &Summary) -> Result<()> { | |||
/// [`iter()`]: #method.iter | |||
/// [`for_each_mut()`]: #method.for_each_mut | |||
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] | |||
#[non_exhaustive] |
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 a breaking change and cannot be done in a point release.
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.
I'll revert it, then; but, should the lint be kept so we can remember for the next major release, or should it be suppressed?
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.
I filed #1835 to track considering it in a point release. I don't have much of an opinion on allowing it, as I don't use clippy. I'm not particularly happy with lots of allow(clippy)
stuff, but if you'd like to add it, that would be fine.
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.
I agree allow
s should be kept to a minimum, hence why I also tried removing existing ones.
If it's being tracked, then I think it's fine to live with the warning—I certainly could, I was more concerned about Clippy's two deny
s in the current codebase.
Also remove `allow(clippy::*)`s where possible
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.
Thanks!
Also remove
#[allow(clippy::*)]
s where possible