w3f / schnorrkel

Schnorr VRFs and signatures on the Ristretto group
BSD 3-Clause "New" or "Revised" License
310 stars 93 forks source link

Add clippy & fmt #24

Closed liamsi closed 5 years ago

liamsi commented 5 years ago

Again, this doesn't change anything substantial only adds clippy and rustfmt to the travis build.

Will close #13 (and disable --features=u32_backendbuild for now).

Nightly builds fail as clippy is currently not available on nightly: https://rust-lang.github.io/rustup-components-history/index.html

The other builds fail due to rustfmt complaining.

burdges commented 5 years ago

There is helpful semantic formatting of method chains and spacing that rustfmt breaks, so those places need to be changed to a more declarative style using explicitly named temporaries first, otherwise the code becomes less readable. It's okay afaik since declarative style usually improves things.

I did briefly looked into disabling anything problematic using options from https://github.com/rust-lang/rustfmt/blob/master/Configurations.md but afaik nothing there cover method chains, except maybe disable_all_formatting. Aside from method chaining, I'd want f( foo ) not to be turned into f(foo) because those spaces say something important.

Anyways, there are many scenarios in which rustfmt makes the wrong stylistic choices, and some in which it breaks code in subtle ways. Worse, they pushed ahead with stabilizing rustfmt without fixing the code breakage, so..

I'm nervous about using rustfmt or encourage anyone else to do so, even after the declarative style improvements it necessitates.

burdges commented 5 years ago

I merged but reverted rustfmt in https://github.com/w3f/schnorrkel/commit/32255cc1acf1688d3b0e90cf9062a2ec7b8b7bb9