search

smart-leds-rs / smart-leds-trait

A trait for implementing effects, modifiers and drivers for programmable leds
Apache License 2.0
2 stars 4 forks source link

feat: add serde feature #10

Closed cyril-marpaud closed 10 months ago

cyril-marpaud commented 1 year ago

This solves https://github.com/smart-leds-rs/smart-leds/issues/16. (I only serialize/deserialize colors programatically as/to JSON so I dont need to be able to deserialize contants like "WHITE" or "BLACK", only previously programatically serialized json like {"r":255,"g":0,"b":0}).

david-sawatzke commented 1 year ago

As is I won't merge this, as serde is much too large of a dependency for an always-on feature with small value for most.

If you just want to serialize/deserialize, you could either choose the option of adding rgb as a dependency with the serde feature enabled to your binary (no changes needed for this repo) or adding the serde feature as an optional feature here (which then also needs to be passed through for the smart-leds crate). I'm not sure about adding the latter option if the former works just as well and is just a bit more obtuse.

cyril-marpaud commented 1 year ago

That's exactly what I did for now, adding the rgb crate as a dependency with the serde feature enabled, and it works. I think it looks ugly though, because my crate does not really use rgb. Let me know if you change your mind about "the latter option", I'll be glad to update my PR then (from my POV, this is how it should be).

cyril-marpaud commented 1 year ago

Finally decided to update the PR anyway.

sajattack commented 1 year ago

I think this is fine so long as it's opt-in.

david-sawatzke commented 10 months ago

Thank you, looks good to me