search

georust / topojson

TopoJSON bindings and utilities for Rust
MIT License
10 stars 8 forks source link

Update with rustfmt, clippy and add to CI #15

Closed pjsier closed 3 years ago

pjsier commented 3 years ago

Should finish #12. Makes updates recommended by clippy and rustfmt and also adds them to GitHub Actions checks

urschrei commented 3 years ago

Does this fail a test on clippy / rustfmt failures, or just warn with details?

pjsier commented 3 years ago

Right now it fails, but I think it would make sense for either of those checks to be a warning instead

urschrei commented 3 years ago

Ah. I ask because there's been some tension regarding formatting requirements (but not Clippy, actually) being seen as a cause of friction / contributor barrier to entry on the actual Rust project, so I was curious. One thing that was pointed out was that the formatting run can be automated to occur before a merge commit, which I like a lot, but have no idea how to do. I like the idea of a warning for either / both tools, though, and we should discuss adopting that more widely if people are amenable.

pjsier commented 3 years ago

That makes sense! I usually have my editor set up to run rustfmt on save, but I can imagine for clippy in particular that might be more difficult.

For running before a merge commit, I think with GitHub Actions now you can have a separate user make changes and then commit them to a branch. I'd be a little worried about automated commits for the formatter, but that isn't really based on much and if it becomes an issue I think we could set that up.

Changing these to warn instead of failing seems like a good next step for now, and I can update that here, thanks for the comments!

pjsier commented 3 years ago

Any other thoughts on this before I merge?