-
Notifications
You must be signed in to change notification settings - Fork 63
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 of the clippy warnings #74
base: master
Are you sure you want to change the base?
Conversation
tempdir is deprecated
As a library, panicking, even in the face of a bug, is a problem. Also, a lot of warnings (mostly `try!` vs `?` and `dyn`) were removed, as part of the edit/rebuild/test cycle.
If Slackware thinks Rust 1.46 is stable enough for everyday use (i.e., building Mozilla) then it should be stable enough for anyone who doesn't track "stable".
`cargo clippy` warned of infinite recursion, so let's heed the warning
This reverts commit 3a8db85. It was applied to the wrong branch by accident.
src/data.rs
Outdated
@@ -20,7 +23,10 @@ impl PartialEq for Data { | |||
(&Data::Bool(ref v0), &Data::Bool(ref v1)) => v0 == v1, | |||
(&Data::Vec(ref v0), &Data::Vec(ref v1)) => v0 == v1, | |||
(&Data::Map(ref v0), &Data::Map(ref v1)) => v0 == v1, | |||
(&Data::Fun(_), &Data::Fun(_)) => bug!("Cannot compare closures"), | |||
(&Data::Fun(_), &Data::Fun(_)) => { | |||
bug!("Cannot compare closures"); |
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.
Is this a bug, or simply always false, i.e. no two closures are equal, even to themselves.
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.
Yeah, I wasn't really sure, so I left it as is. I can see an argument where closures aren't reflexive (and they don't implement PartialEq
, which is a hint that the Rust developers agree), but I can also see how users might reasonably assert that some closures are be equal.
I think we run into the halting problem, and overall asserting that they're not equal is the right thing to do, but logging a message might tell the middleware developer there's a problem and that's why the template isn't firing. Since bug!
is now a wrapper around error!
I think it's okay as is.
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.
One minor comment I don't feel strong about.
I fixed the merge conflicts in this branch and in #76 (folding it into this). Both were spurious, caused by the files being touched after the branches were made. |
And this pulls in #71 and cleans up some lint in it. Note that |
I suspect we can move to Rust 2018 w/o changing the API, but let's make that a different PR. |
This is based on top of #73 which also got rid of a bunch of warnings, but goes a bit further.
It leaves in two warnings:
rust-mustache/src/template.rs
Line 237 in 94afdba
rust-mustache/src/compiler.rs
Line 56 in 94afdba
I can't tell if this is the false negative from https://rust-lang.github.io/rust-clippy/master/index.html#map_entry or not, so I left it alone.