georust / geo-svg

A rust library to generate SVGs for geo-types
5 stars 4 forks source link

Fix Color::Hex and add tests #10

Closed vargad closed 6 months ago

RobWalt commented 6 months ago

Thanks for contributing that! I love the included tests.

I'm afraid there's still room for confusion for users since 0xFFF will correspond to the color #000FFF now.

Anyways. We can always fix that in a follow up PR. I'll leave it up to you. Let me know if you want to merge this or keep working on it.

RobWalt commented 6 months ago

Just writing down my thoughts so that they're stored somewhere:

It would probably be best to create a public wrapper type for Color and make the current Color enum private. The wrapper type would then implement various new_* methods like new_named, new_hex, ... you get the drift.

Additionally I think we need to accept a &str argument for new_hex(...) as a raw u32 is ambiguous. As explained in the other comment, #FFF and #000FFF are two different colors although they share the same "raw" u32 bit pattern.

vargad commented 6 months ago

I think this should be merged as it is a bugfix, that stands on it's own. Without this change the Color::Hex cannot be used.

To me the value of hex over rgb is that it's shorter and easier to read. Indeed, for the rust compiler 0x000fff is the same as 0xfff. I agree that making the enum private and adding construct methods would prevent any misuse. Might be preferable not to parse the hex string runtime. For the hex could have a new_hex(r: u8, g: u8, b: u8) method, so misuse is not possible and have a hex!(fff) macro for shorthand alongside with macros for each type.

RobWalt commented 6 months ago

I released a patched version 👍🏼