search

colored-rs / colored

(Rust) Coloring terminal so simple you already know how to do it !
Mozilla Public License 2.0
1.74k stars 84 forks source link

Use the rgb crate instead of own struct #174

Open RuboGubo opened 4 months ago

RuboGubo commented 4 months ago

closes #173

It might be an idea to add a feature flag for this, but it's up to you, just shout if you want me to add it.

RuboGubo commented 4 months ago

Should add that this is a breaking change, so you should prob increment the major version of the crate, up to you though

spenserblack commented 4 months ago

Oh dear, I got mixed up and was thinking this PR was targeting a completely different project that I maintain. Sorry, my comments might not have all been applicable.

Rather than remove an existing type, it might be better to implement From<T> so that these two types can easily be converted back and forth. If we really want to remove CustomColor, it can be marked as deprecated so that it's not breaking, but users are encouraged to switch. If we want users to use Rgb.

RuboGubo commented 4 months ago

I think migrating to Rgb is the right idea, just to make the whole ecosystem work more smoothly. In terms of making CustomColor exist, i re-added it but deprecated.

Also the Rgb struct has a lot of extra helper functions that the CustomColor struct doesn't have.

spenserblack commented 4 months ago

For completeness I think you might be able to also implement From<Rgb<u8>> for CustomColor. Additionally, I think you might be able to expand impl From<CustomColor> for Rgb<u8> to impl<T: From<u8>> From<CustomColor> for Rgb<T>, which would allow conversion for Rgb<u16>, Rgb<u32>, etc.

Before you put in any more work, though, it might be good to wait and see what the other maintainers think about introducing the rgb crate.

RuboGubo commented 4 months ago

I think given that no-one has responded, I'll get the code cleaned up and passing, and then we can go from there