-
Notifications
You must be signed in to change notification settings - Fork 40
add missing impl Error
s (breaking change)
#130
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
base: main
Are you sure you want to change the base?
Conversation
just spotted in the diff that |
5ea31ba
to
34a6a89
Compare
34a6a89
to
4ec7cbc
Compare
i've undone the re-formatting here and instead did it for all crates in #131 where i also added a CI check for it |
|
||
/// A conversion trait to convert a user error into a base Kind type | ||
pub trait AsWireTxErrorKind { | ||
pub trait AsWireTxErrorKind: core::error::Error { |
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.
This and line 150 makes this a breaking change according to cargo semver-checks
:
james@magician ➜ postcard-rpc git:(add-missing-Error-impls) cargo semver-checks
Building postcard-rpc v0.11.12 (current)
Built [ 9.633s] (current)
Parsing postcard-rpc v0.11.12 (current)
Parsed [ 0.016s] (current)
Building postcard-rpc v0.11.12 (baseline)
Built [ 9.127s] (baseline)
Parsing postcard-rpc v0.11.12 (baseline)
Parsed [ 0.015s] (baseline)
Checking postcard-rpc v0.11.12 -> v0.11.12 (no change)
Checked [ 0.020s] 153 checks: 152 pass, 1 fail, 0 warn, 11 skip
--- failure trait_added_supertrait: non-sealed trait added new supertraits ---
Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_added_supertrait.ron
Failed in:
trait postcard_rpc::server::AsWireRxErrorKind gained Error in file /Users/james/personal/postcard-rpc/source/postcard-rpc/src/server/mod.rs:150
trait postcard_rpc::server::AsWireTxErrorKind gained Error in file /Users/james/personal/postcard-rpc/source/postcard-rpc/src/server/mod.rs:97
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 22.978s] postcard-rpc
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.
are these traits supposed to be implemented by anyone outside of postcard-rpc
? if not then it wouldn't be breaking, right?
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.
They are public, so they can be implemented by anyone: https://docs.rs/postcard-rpc/latest/postcard_rpc/server/index.html#traits
I'm overall fine with this, though this is a breaking change. We might have one soon anyway when the |
otherwise i can split it up and just add the |
Feel free to break it up if you'd like! Happy to merge for |
i've split it to #132 for now |
use `thiserror` to do this as it's already used in the crate. this updates it from v1 to v2 and also uses it in `no_std` environments as that's now supported by `thiserror`. this was missing and thus it wasn't possible to use e.g. `HostErr` in a `#[from]` with `thiserror`.
4ec7cbc
to
75d0643
Compare
rebased on top of #132 |
impl Error
simpl Error
s (breaking change)
(updated the title so I don't forget and merge this without thinking about it :) ) |
use
thiserror
to do this as it's already used in the crate. this updates it from v1 to v2 and also uses it inno_std
environments as that's now supported bythiserror
.this was missing and thus it wasn't possible to use e.g.
HostErr
in a#[from]
withthiserror
.