Skip to content

Conversation

rursprung
Copy link
Contributor

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.

@rursprung
Copy link
Contributor Author

just spotted in the diff that cargo fmt --all formatted a bit more than just my code... i hope that's ok? probably should add a CI check for proper formatting?

@rursprung
Copy link
Contributor Author

just spotted in the diff that cargo fmt --all formatted a bit more than just my code... i hope that's ok? probably should add a CI check for proper formatting?

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 {
Copy link
Owner

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

Copy link
Contributor Author

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?

Copy link
Owner

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

@jamesmunns
Copy link
Owner

I'm overall fine with this, though this is a breaking change. We might have one soon anyway when the embassy-executor changes are worked through, so maybe we leave this PR open for a bit and we can merge it when getting ready for a v0.12.

@rursprung
Copy link
Contributor Author

I'm overall fine with this, though this is a breaking change. We might have one soon anyway when the embassy-executor changes are worked through, so maybe we leave this PR open for a bit and we can merge it when getting ready for a v0.12.

otherwise i can split it up and just add the impl Error for HostErr (which is the one i actually care about) in a separate PR to get that in as a non-breaking change?

@jamesmunns
Copy link
Owner

Feel free to break it up if you'd like! Happy to merge for v0.11.x as long as cargo semver-checks is happy, otherwise happy to merge as part of v0.12 in the nearish future.

@rursprung
Copy link
Contributor Author

Feel free to break it up if you'd like! Happy to merge for v0.11.x as long as cargo semver-checks is happy, otherwise happy to merge as part of v0.12 in the nearish future.

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`.
@rursprung rursprung force-pushed the add-missing-Error-impls branch from 4ec7cbc to 75d0643 Compare August 4, 2025 10:11
@rursprung
Copy link
Contributor Author

rebased on top of #132

@rursprung rursprung requested a review from jamesmunns August 4, 2025 10:11
@jamesmunns jamesmunns changed the title add missing impl Errors add missing impl Errors (breaking change) Aug 4, 2025
@jamesmunns
Copy link
Owner

(updated the title so I don't forget and merge this without thinking about it :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants