Make CI fail if Clippy finds any warnings.

[crsaracco]

Fail the CI tests if Clippy reports any warnings.

[askeksa]

A few comments regarding the float comparison changes.

The rest of the changes seem pretty reasonable. At least they don't seem to break anything. :)

[crsaracco]

@askeksa changes SGTM, I'll re-visit them and fix after #102 is merged in.

[askeksa]

Actually, when looking closer, it looks to me like none of the approximate float comparisons are warranted. They are either checking if some value has changed since last time, checking against a specific value set in a test, or comparing with a value obtained by summing some integers. In all of these cases, exact comparison is the right thing to do.

[crsaracco]

Actually, when looking closer, it looks to me like none of the approximate float comparisons are warranted. They are either checking if some value has changed since last time, checking against a specific value set in a test, or comparing with a value obtained by summing some integers. In all of these cases, exact comparison is the right thing to do.

Agreed. I think I was just blindly following clippy for that lint when I made this PR.

Reverted back to strict compares, added #[allow(clippy::float_cmp)] for those tests, and removed the test_util module I added since the functions weren't being used anymore.

[askeksa]

Many of the Clippy suggestions are good, but some are really bad, making me wonder if enforcing no Clippy warnings across the repo is such a good idea after all. At least without a generous, crate-wide list of exceptions (which, it seems, is not yet possible).

Many of the changes in this PR are still fine, and could go into a "Fix some Clippy warnings" commit.

[crsaracco]

Many of the Clippy suggestions are good, but some are really bad, making me wonder if enforcing no Clippy warnings across the repo is such a good idea after all. At least without a generous, crate-wide list of exceptions (which, it seems, is not yet possible).

I don't know, I think the Clippy suggestions are good enough to warrant enforcement in the general case, especially in the context of an open source project where anyone can make a PR. Even moreso when other projects in the ecosystem use Clippy to help identify ecosystem-wide issues. We have historically been terrible at checking the travis output to see if we broke any Clippy lints, so I'd rather enforce them up-front.

I think it's easy enough to chuck on a #[allow(clippy::whatever)] with justifications in the code review (I would even like the justifications in the comments, but anyways...)

Finally, if a particular lint is causing us too much trouble for what it's worth, we can decide to ignore it crate-wide in the CI file, like I did for "unreadable_literal"s in this PR.

[askeksa]

There was one literal left (the unique_id in transfer_and_smooth) that I think should be reverted. And then float_cmp and needless_range_loop added to the Clippy exceptions. Then it looks good. :)

[askeksa]

And yes, I do agree that it makes sense to turn the warnings on, with appropriate exceptions. I think we should have the three mentioned. Two of them because they are "causing us too much trouble for what it's worth" as you say, and one because it is buggy (which together with just one false positive warrants disabling, IMO).

[crsaracco]

So just to be explicit, the three crate-wide ignored warnings are going to be (as of this PR):

• clippy::unreadable_literal
• clippy::needless_range_loop
• clippy::float_cmp

Right?

[askeksa]

Yes, exactly. Good idea to add a comment about the buggy needless_range_loop, btw.