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

Use the rgb crate instead of own struct #174

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

RuboGubo
Copy link

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.

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

TBH I don't have a strong opinion one way or the other, but it could be nice to use a "standard" type instead of our own.

Looks like some tests need to be fixed before this can be merged.

src/lib.rs Outdated Show resolved Hide resolved
@RuboGubo
Copy link
Author

RuboGubo commented Jul 16, 2024

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

spenserblack
spenserblack previously approved these changes Jul 16, 2024
@@ -20,13 +20,23 @@
//! format!("{:30}", "format works as expected. This will be padded".blue());
//! format!("{:.3}", "and this will be green but truncated to 3 chars".green());
//!
//! Custom colours are implemented using the `rgb` crate, which is re-exported for
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we using UK English or American English in the rest of our docs? 😆

Copy link
Author

@RuboGubo RuboGubo Jul 16, 2024

Choose a reason for hiding this comment

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

well if UK English is an option....

@spenserblack
Copy link
Collaborator

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.

@spenserblack spenserblack dismissed their stale review July 16, 2024 21:01

Potentially invalid approval

@RuboGubo
Copy link
Author

RuboGubo commented Jul 16, 2024

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
Copy link
Collaborator

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
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

Support using the rgb crate
2 participants