Closed lloydmeta closed 6 years ago
frunk
should also do this, not just core.
But hmmm... I dont think it will be possible to have the serde
feature of frunk toggle the serde
feature of core when using this hack. Let me think about this...
Here's how num
does it:
https://github.com/rust-num/num/blob/master/Cargo.toml
Basically, num
itself doesn't contain anything, so it doesn't need serde as a dependency. That allows "serde" to be used as a feature name.
For frunk
to be able to do that, everything in frunk that doesn't come from frunk-core
would need to be split out into a third crate. That's kind of a big change! :P
Still looking for a better alternative...
Okay, so it looks like having the features be disconnected from each other is already the status quo for frunk, so this is my recommendation for now:
frunk-core
: (note: this was already true before this PR, but I think it is worth documenting now)[dependencies]
frunk = { version = "0.1.36", features = ["serde"] }
frunk-core = { version = "0.1.36", features = ["serde"] }
@ExpHP Aaah right; good point. I've made the same change to frunk and updated the readme. Let me know what you think.
Hm, that's funny. I didn't realize this earlier, but: judging from how the second commit doesn't touch any .rs
files, I guess there isn't actually anything in frunk
itself that has serde derives? :stuck_out_tongue:
Okay, so technically you can do what num
does, and add a serde
feature to frunk
that activates the feature "frunk-core/serde"
. The advantage of this would be that a user doesn't need to depend on frunk-core
explicitly.
Or: what you just commited is also fine! Even if it does seem a bit silly at the moment, it has the advantage that (non-core) frunk
will be able to backwards-compatibly add things that use serde in the future.
(if you go with option 2 then LGTM)
Yeah, frunk
doesn't at the moment, just thought it might be nice to have something there in case it did in the future. I think I'll go with option 2 for the added backwards-compatibility.
Thanks for the review !
Closes #97