Skip to content
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

Serde feature enabled by default (through std feature) #30

Open
phil-opp opened this issue May 7, 2020 · 2 comments
Open

Serde feature enabled by default (through std feature) #30

phil-opp opened this issue May 7, 2020 · 2 comments

Comments

@phil-opp
Copy link
Contributor

phil-opp commented May 7, 2020

The serde feature is active by default because the std feature depends on it:

mqttrs/Cargo.toml

Lines 17 to 22 in 17cc76e

[features]
default = ["std"]
# Implements serde::{Serialize,Deserialize} on mqttrs::Pid.
derive = ["serde"]
std = ["bytes/std", "serde/std"]

This makes it impossible to build the library with only the std feature, but not the serde feature. Also, the Readme states that you have to add the derive feature in order to use serde, which is not required.

Ideally, we would want to enable the serde/std feature only if both the std and derive features are active, but I don't know if that's possible.

(This is not a real problem for me at the moment, I just though that this looks like a bug, so I decided to report it. Thanks for creating this library, it looks very promising from a first look!)

@vincentdephily
Copy link
Collaborator

Good catch, thanks. I pushed a quick fix to https://github.com/00imvj00/mqttrs/tree/fix_feature_hierarchy but didn't test it beyond looking at cargo build and cargo tree with various features enabled.

I'm not sure how to properly unittest feature-dependant code. Maybe using an examples folder ? How do I test for the absence of a feature ?

@phil-opp
Copy link
Contributor Author

Great, thanks! One possible approach for testing could be to create an example that fails to compile if the serde feature is enabled (e.g. using compile_error!). Then you can try to compile it with the std feature to ensure that the serde feature is not enabled implicitly.

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

No branches or pull requests

2 participants