-
Notifications
You must be signed in to change notification settings - Fork 57
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
non_exhaustive structures #140
Conversation
bcd17ca
to
bd42596
Compare
Swapping to non_exhaustive LGTM, should this PR wait until #139 so it doesn't also merge the strict stuff? |
At your discretion. |
I'm also fine in principle, though I'm not quite clear on the motivation to bump the MSRV just to replace manual non-exhaustive implementation. |
At this point I think 1.40 is old enough that it's nice to be able to clean up the code to use non_exhaustive. Especially since this crate tends to be more "tooling" than "end-user" I'm really not too worried about the MSRV going to 1.40. |
I'm not worried about the MSRV bump. I'm simply not a big fan of bumping versions just for cleanup... |
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.
We need to bump the version we run clippy under too to support anyhow's clippy config, and this helps tidy up the codebase, so on balance I think let's go for it (and merge #139 as part of this PR).
bors merge
Ah, this line will need updating before bors sees the tests as passing: https://github.com/rust-embedded/svd/blob/master/.github/bors.toml#L6 |
bors cancel |
Canceled. |
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.
bors merge
Ah, this failure wasn't being reported but explains why we're not seeing any https://github.com/rust-embedded/svd/actions/runs/695451779 In particular bors cancel |
31fd04c
to
91f7976
Compare
Canceled. |
Rebased |
What is wrong with |
It's still complaining about this line in anyhow: {"reason":"compiler-message","package_id":"anyhow 1.0.40 (registry+https://github.com/rust-lang/crates.io-index)","target":{"kind":["lib"],"crate_types":["lib"],"name":"anyhow","src_path":"/home/runner/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/anyhow-1.0.40/src/lib.rs","edition":"2018","doctest":true},"message":{"rendered":"error: error reading Clippy's configuration file but I don't know why, or why it has worked occasionally... The line seems to be valid Clippy: https://github.com/rust-lang/rust-clippy#specifying-the-minimum-supported-rust-version I think it's a relatively new Clippy feature: rust-lang/rust-clippy#6097 so my guess was that for some reason the clippy check is being run with an older version of Rust? |
cfe867a
to
8d242de
Compare
Switched to use last stable clippy |
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 last thing, we should probably add test-strict and clippy_check to the bors config so it requires that they pass.
Co-authored-by: Adam Greig <adam@adamgreig.com>
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.
bors merge
Build succeeded: |
Up MSRV to 1.40