Export color constants as associated constants on Color

[sunjay]

When this crate was first written back in 2017, you couldn't have associated constants on structs, you had to use modules. Today, we can do that using the syntax:

impl Color {
    pub const BLACK: Color = Color {red: 0.0, green: 0.0, blue: 0.0, alpha: 1.0};
}

Now that this is possible, it would be nice to get rid of the color module altogether and just use associated constants instead. This has the benefit of allowing people to use color constants directly from Color instead of having to import a separate module.

A potential downside of this is that we may end up with an overwhelming number of constants listed in the documentation for Color. We can solve this in one of two ways:

1. Add a link at the top/bottom of the docs for Color that allows people to jump past the constants
2. Leave the constants in extended where they are and maybe rename the module to extra_colors when re-exporting the module from lib.rs

To-Do

• [x] Change the color_consts! macro to generate the constants within an impl Color block instead of just at the top-level of the module
• [x] Potentially get rid of the extended module (see above) and join the two invocations of color_consts! into a single invocation
• [x] Rename all the references to color::CONST_NAME to Color::CONST_NAME
• [x] Remove the unused imports to color
• [x] Make the color module private
• [x] Move the documentation for color to the Color struct
• [x] Go through the documentation and make sure nothing refers to the color module anymore
• [x] Make sure the tests pass

Mentoring Instructions

To change the macro, you'll need to make the following changes:

diff --git a/src/color.rs b/src/color.rs
index 7204bbe..0a7121e 100644
--- a/src/color.rs
+++ b/src/color.rs
@@ -1199,16 +1199,18 @@ impl<'a> From<&'a str> for Color {

 macro_rules! color_consts {
     ($($name:expr, $id:ident, ($r:expr, $g:expr, $b:expr, $a:expr);)*) => {
-        $(
-            #[doc = $name]
-            pub const $id: Color = Color {red: $r, green: $g, blue: $b, alpha: $a};
-        )*
-
+        impl Color {
             /// A list of all of the colors
             pub static COLORS: &[Color] = &[$($id, )*];
             /// A list of all of the color names
             pub static COLOR_NAMES: &[&str] = &[$($name, )*];

+            $(
+                #[doc = $name]
+                pub const $id: Color = Color {red: $r, green: $g, blue: $b, alpha: $a};
+            )*
+        }
+
         pub(crate) fn from_color_name(s: &str) -> Option<Color> {
             match s {
                 $(

After that, the other steps in the To-Do list are fairly mechanical. You are going to get a ton of compiler errors which you will have to fix. I recommend using find/replace as much as you can for this. Your editor may also have fancy refactoring tools you can use to make this easier.

Questions are welcome! Feel free to ask in the comments below or on Zulip.

[noeddl]

Hi @sunjay! I currently have some time over for pleasure hacking and remembered contributing to this project was so much fun the last time so I decided to come back and see if I can contribute some more. :slightly_smiling_face:

I started playing around with this issue and changed the color_consts! macro in the way you suggested above. Unfortunately, the compiler complains about the definitions of COLORS and COLOR_NAMES inside of the impl Color block because associated `static` items are not allowed. Do you have any ideas on how to deal with this?

[sunjay]

@noeddl Thanks for taking the time to work on this! Since static variables aren't allowed, let's expose those values as associated functions on Color (methods with no self argument). We can have:

impl Color {
    fn all_colors() -> &'static [Color] { ... }
    fn all_color_names() -> &'static [&'static str] { ... }
}

(If possible, omit the 'static from the signatures to keep things easy to read. If the compiler complains, it is fine to keep them there.)

[noeddl]

If we want to get rid of the extended module and merge the two invocations of color_consts! into one there is the problem that some constants exist both in the color and the extended module but with different definitions.

For example, GREY has the values (128.0, 128.0, 128.0, 1.0) in color but (146.0, 149.0, 145.0, 1.0) in extended. Which of the definitions should we use? My first idea was to keep the ones from https://xkcd.com/color/rgb/ that you used for the extended module. But this would also change the look of existing uses of constants from color. So maybe it would be better to go for the second option and keep around the extended module (or some other way of keeping the distinction between constants that originally where located in color vs. the ones in extended)?

[sunjay]

@noeddl Backwards comparability is important. Thanks for considering that!

If the extended module duplicates any of the color names, that's likely because I made a mistake in defining the colors there (especially if the colors have different values). We should remove the duplicates.

Luckily, most people don't use the color constants directly. They use the color names as strings, e.g. "gray". The crate uses the color constants internally to map color names to their values. We won't break too many peoples' code as long as we keep the version of the color that the string currently resolves to the same. (See the From<&str> impl for Color)

Note: Do keep in mind the comments in the original issue about how merging the extended module may create an overwhelming number of constants in the documentation. If we do decide to merge in the extended module, let's do it in a follow up PR so we don't waste any work if we decide it's too much.

Thanks again for taking the time to work on this!

[noeddl]

Okay, thanks for the clarification! So I keep the extended module for now but remove the duplicates in it.

But then we actually need two different macros to generate the color constants, right? One version of color_consts! that behaves in the current way to be invoked from inside the extended module and one version that is adaped in the way you showed in the issue to turn the top-level constants in color into associated constants of Color. (At least that's what I concluded after unsuccessfully trying to solve it with one and the same macro.)

[sunjay]

@noeddl That is correct. Feel free to create a copy of color_consts! if you need to. I am fine with having two versions while we figure out how things should look.