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 of the clippy warnings #74

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

therealbstern
Copy link
Contributor

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:

ninjabear and others added 11 commits March 3, 2020 15:40
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");
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@jolhoeft jolhoeft left a 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.

@therealbstern
Copy link
Contributor Author

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.

@therealbstern
Copy link
Contributor Author

And this pulls in #71 and cleans up some lint in it.

Note that rust-mustache still isn't using Rust 2018, which causes a big lint barf at the beginning, but moving to 2018 might be an API change, so I'm deferring that.

@jolhoeft
Copy link
Member

I suspect we can move to Rust 2018 w/o changing the API, but let's make that a different PR.

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.

5 participants