Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Allow enabling serde/std without also requiring serde_cbor/std to be enabled #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kylefleming
Copy link

Currently serde_cbor's std feature must be enabled if serde/std is also enabled, even if serde/std is enabled separately by another crate somewhere else in the dependency tree.

However, this doesn't necessarily need to be the case. serde provides a StdError type in order to get around this (see serde-rs/serde#1620). This is a type provided by serde as a re-export of std::error::Error when serde/std is enabled and as an export of a custom serde::std_error::Error type with the same interface as std::error::Error when serde/std is not enabled. By conforming to serde::ser::StdError instead of std::error::Error, serde_cbor can rely on the std/no_std state of serde rather than requiring that both libraries' std feature be either both enabled or both disabled.

This allows serde_cbor to be in no_std even if serde/std is enabled. While this might not make a ton of sense at first glance, this change would lead to a much more robust dependency mechanism when serde and serde_cbor are involved, by allowing a crate to depend on serde_cbor with default features disabled and not have to worry if another crate (maybe even higher up in the dependency graph) enables serde/std completely independently. Ideally, the ultimate goal is that features are additive and not dependent on the state of other features laterally in the dependency graph.

This PR does 2 things:

  • Changes the type of the std error that serde_cbor::Error conforms to from std::error::Error to serde::ser::StdError.
  • Adds a line to .travis.yml to test building with serde/std enabled and serde_cbor's default features disabled in order to test that this configuration is buildable.

Copy link
Contributor

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

@stevenroose
Copy link
Contributor

I'm not sure who's in charge atm for merging in this repo. But I think this is a good change.

@rjwalters
Copy link

it seems that only @pyfisch has admin rights

@kylefleming
Copy link
Author

This was auto-closed completely unintentionally.

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

Successfully merging this pull request may close these issues.

None yet

3 participants