Implement `FromStr` method for `Rgb<S, u8>` #157

mswift42 · 2019-11-30T11:34:55Z

This enables converting hex colors like '#00ddff' or '#fff' to Rgb.

this closes #148.

Ogeon

Thanks for doing this! There are just a few points that will need to be changes, primarily to maintain the #[no_std] support, but also a couple of other details.

Ogeon · 2019-12-01T15:03:06Z

palette/src/rgb/rgb.rs

@@ -2,6 +2,7 @@ use core::any::TypeId;
 use core::fmt;
 use core::marker::PhantomData;
 use core::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign};
+use std::str::FromStr;


This need to be changed to get it from core instead, for the #[no_std] support to work.

Suggested change

use std::str::FromStr;

use core::str::FromStr;

Ogeon · 2019-12-01T15:15:47Z

palette/src/rgb/rgb.rs

@@ -953,14 +954,50 @@ where
    }
 }

+impl FromStr for Rgb {


While this may be the more convenient option for many cases, I would still prefer to implement it for Rgb<Srgb, u8>. I think that's less surprising than only having it for f32. Ideally both would be supported, but then what about if it's a u16 hex string? Or even larger? So, in the end I think it's better to start small, as you have, but for Rgb<Srgb, u8>.

Thinking some more about this, I don't think there's any reason to limit it to only Srgb.

Suggested change

impl FromStr for Rgb {

impl<S: RgbStandard> FromStr for Rgb<S, u8> {

It would also be great to have one for Alpha<Rgb<S, u8>>.

Ogeon · 2019-12-01T15:22:05Z

palette/src/rgb/rgb.rs

@@ -953,14 +954,50 @@ where
    }
 }

+impl FromStr for Rgb {
+    type Err = Box<dyn std::error::Error>;


This will have to change to something that isn't Boxed, due to the #[no_std] support requirement. A simple error type like

enum FromHexError { ParseIntError(ParseIntError), HexFormatError(&'static str), // Like "invalid hex code length" or "unexpected hex code prefix" }

with all the impls it would need, would do the trick.

I struggle with this. Sorry, I am fairly new to rust, but there is no Error type in core only in std.

Do you have a suggestion how to do this ?

Ah, ok. No worries! :) You are right, it seems like the Error trait is only in std. In that case we will have to make use of conditional compilation if we want to use it (it's good practice to implement Error, so we should). #[no_std] is a particularly prickly feature. If we use std anywhere, unconditionally, it's all out the window, and it's hard to notice unless it's used on a platform that doesn't support the std library. I have set up a CI test to detect it, and that's what made this PR fail the tests.

For this case we have to give up the convenience of having the boxed trait in either case, since Box is also not available, so the first step would be to add an enum like the one I suggested and change this line like this:

Suggested change

type Err = Box<dyn std::error::Error>;

type Err = FromHexError;

That way, the parsing will work both with and without std.

Then the FromHexError type will need some traits (I would look at ParseIntError for inspiration), including Error. That's where we will have to check if the library user is allowing the std library:

#[cfg(feature = "std")] // Checks if the "std" feature from Cargo.toml is enabled impl std::error::Error for FromHexError { // `std` is safe to use in this `impl` item, because of the `cfg` attribute. // It will not be included at all if the "std" feature is disabled. // // ... }

The same goes for any other trait that is not in core.

Ogeon · 2019-12-01T15:27:46Z

palette/src/rgb/rgb.rs

+impl FromStr for Rgb {
+    type Err = Box<dyn std::error::Error>;
+
+    // parses a color hex code of format '#ff00bb' or '#abc' into a


It mentions "#" being part of the format, but never checks that it's there. Better to be strict and relax the rules later, rather than the opposite. Besides, it would also be technically possible to support strings without the "#". Maybe that should be added too?

Also, capital "P" in "parses". 🙂

mswift42 · 2019-12-08T16:25:43Z

awesome, thanks.

…

On Sun, 8 Dec 2019 at 16:07, Erik Hedvall ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In palette/src/rgb/rgb.rs <#157 (comment)>: > @@ -953,14 +954,50 @@ where } } +impl FromStr for Rgb { + type Err = Box<dyn std::error::Error>; Ah, ok. No worries! :) You are right, it seems like the Error trait is only in std. In that case we will have to make use of conditional compilation if we want to use it (it's good practice to implement Error, so we should). #[no_std] is a particularly prickly feature. If we use std anywhere, unconditionally, it's all out the window, and it's hard to notice unless it's used on a platform that doesn't support the std library. I have set up a CI test to detect it, and that's what made this PR fail the tests. For this case we have to give up the convenience of having the boxed trait in either case, since Box is also not available, so the first step would be to add an enum like the one I suggested and change this line like this: ⬇️ Suggested change - type Err = Box<dyn std::error::Error>; + type Err = FromHexError; That way, the parsing will work both with and without std. Then the FromHexError type will need some traits (I would look at ParseIntError for inspiration), including Error. That's where we will have to check if the library user is allowing the std library: #[cfg(feature = "std")] // Checks if the "std" feature from Cargo.toml is enabled impl std::error::Error for FromHexError { // `std` is safe to use in this `impl` item, because of the `cfg` attribute. // It will not be included at all if the "std" feature is disabled. // // ... } The same goes for any other trait that is not in core. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#157?email_source=notifications&email_token=AARK7OUQWK266FL355XDTITQXULUTA5CNFSM4JTGHB6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOLE4AY#discussion_r355196778>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARK7OXGSQFVW2DIBJKK6WLQXULUTANCNFSM4JTGHB6A> .

Ogeon · 2019-12-18T21:13:45Z

palette/src/rgb/rgb.rs

+    fn from_str(hex_code: &str) -> Result<Self, Self::Err> {
+        match hex_code.len() {
+            3 | 4 | 6 | 7 => {
+                let (red, green, blue, factor) = if hex_code.len() == 3 {


I'm just stopping by to see how it's going. Looks promising! I would just recommend to either drop this if-else chain and only use the match, or the other way around. Both would work. As it is now, you are checking the length twice, at least. 🙂

An additional idea, to halve the amount of cases, is to check for the existence of # at the beginning first. If it's there, you slice it away. That way, you are down to only two possible lengths when parsing the components.

Yes, I've done it this way. (December is a really busy month, :)

Indeed, it is. But don't feel like you have to rush it or anything. :)

Ogeon · 2019-12-20T17:47:46Z

I think this looks really good! The only addition I think would be good to have would be to implement Error for FromHexError, as we talked about. 🙂 After that I'm definitely prepared to merge this, if you are happy with it. But, once again, take your time and let me know if anything is still unclear.

Ogeon · 2020-01-05T10:31:48Z

palette/src/rgb/rgb.rs

+                write!(f, "ParseIntError: {}", e)
+            }
+            FromHexError::HexFormatError(s) => {
+                write!(f, "HexFormatError: {}, please use format '#fff', 'fff', '#ffffff' or\
+                'ffffff'.", s)


I think it would be better to skip the ParseIntError: and HexFormatError: prefixes. I don't think they will be that interesting to someone who would read the error message, to be honest. They just want to know what went wrong and how to fix it. 🙂

Ogeon · 2020-01-05T10:34:40Z

This looks really nice! Well done! There's just that detail with the error messages. Also, I think the common convention in general is to just have a plain sentence without an initial capital letter, so there's that too.

mswift42 · 2020-01-05T14:50:57Z

Ok, done, and a happy new year to you 🍾

Ogeon · 2020-01-05T17:27:38Z

Thanks and happy new year to you too! Oh, I forgot, you may want to squash those commits. Just to keep the history a bit shorter.

Ogeon · 2020-01-18T19:09:17Z

Sorry, I didn't see the notification for the squash. It must have gotten lost somewhere...

Thanks you!

bors r+

157: implement FromStr method for Srgb<f32>. r=Ogeon a=mswift42 This enables converting hex colors like '#00ddff' or '#fff' to Rgb. this closes #148. Co-authored-by: mswift42 <martin.haesler@gmail.com>

bors · 2020-01-18T19:25:40Z

Build succeeded

continuous-integration/travis-ci/push

Ogeon requested changes Dec 1, 2019

View reviewed changes

Ogeon reviewed Dec 18, 2019

View reviewed changes

Ogeon reviewed Jan 5, 2020

View reviewed changes

Ogeon approved these changes Jan 5, 2020

View reviewed changes

Implement FromStr for RGB.

4b4e0a4

mswift42 force-pushed the master branch from a787bb8 to 4b4e0a4 Compare January 6, 2020 10:37

bors bot added a commit that referenced this pull request Jan 18, 2020

Merge #157

0279591

157: implement FromStr method for Srgb<f32>. r=Ogeon a=mswift42 This enables converting hex colors like '#00ddff' or '#fff' to Rgb. this closes #148. Co-authored-by: mswift42 <martin.haesler@gmail.com>

bors bot merged commit 4b4e0a4 into Ogeon:master Jan 18, 2020

Ogeon changed the title ~~implement FromStr method for Srgb<f32>.~~ Implement FromStr method for Srgb<f32> Jul 4, 2021

Ogeon changed the title ~~Implement FromStr method for Srgb<f32>~~ Implement FromStr method for Rgb<S, u8> Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `FromStr` method for `Rgb<S, u8>` #157

Implement `FromStr` method for `Rgb<S, u8>` #157

mswift42 commented Nov 30, 2019

Ogeon left a comment

Ogeon Dec 1, 2019

Ogeon Dec 1, 2019 •

edited

Loading

Ogeon Dec 7, 2019

Ogeon Dec 1, 2019

mswift42 Dec 8, 2019

Ogeon Dec 8, 2019

Ogeon Dec 1, 2019

mswift42 commented Dec 8, 2019 via email

Ogeon Dec 18, 2019

Ogeon Dec 18, 2019

mswift42 Dec 20, 2019

Ogeon Dec 20, 2019

Ogeon commented Dec 20, 2019

Ogeon Jan 5, 2020

Ogeon commented Jan 5, 2020

mswift42 commented Jan 5, 2020

Ogeon commented Jan 5, 2020

Ogeon commented Jan 18, 2020

bors bot commented Jan 18, 2020

	impl FromStr for Rgb {
	impl<S: RgbStandard> FromStr for Rgb<S, u8> {

	type Err = Box<dyn std::error::Error>;
	type Err = FromHexError;

Implement FromStr method for Rgb<S, u8> #157

Implement FromStr method for Rgb<S, u8> #157

Conversation

mswift42 commented Nov 30, 2019

Ogeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mswift42 commented Dec 8, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon commented Dec 20, 2019

Choose a reason for hiding this comment

Ogeon commented Jan 5, 2020

mswift42 commented Jan 5, 2020

Ogeon commented Jan 5, 2020

Ogeon commented Jan 18, 2020

bors bot commented Jan 18, 2020

Build succeeded

Implement `FromStr` method for `Rgb<S, u8>` #157

Implement `FromStr` method for `Rgb<S, u8>` #157

Ogeon Dec 1, 2019 •

edited

Loading