Color class

[MrGibbage]

Looking through the Color documentation, I see this:

# You can make your own colors. Saturation and value are 100 by default.
my_dark_green = Color(h=125, s=80, v=30)

# You can also add colors to the builtin colors.
Color.MY_DARK_BLUE = Color(h=235, s=80, v=30)

The second way, adding custom colors to the built-in class, feels strange to me. I can't think of another API that I have ever used where the user can modify built in class attributes this way. I mean, it works here, and maybe this is kind of a python thing (I have only been programming python for a couple of years... previously java and c++), but to me it just doesn't seem right from a code organization point of view.

Is there a good reason to use one method over the other that I may not be thinking about?

By the way, two other points:

1. I wonder if there should be another set of built in colors. I like the fully saturated colors that are included. Those work great for setting the light colors. But they are not very good for the colors the sensor sees. We do something like this (using the second method from above):

Color.SENSOR_GREEN = Color(h=156, s=66, v=66)  # h=120,s=100,v=100

This was determined by testing, using our competition robot.
And then we have a conversion such that when the sensor sees SENSOR_GREEN, it can then turn the hub button Color.GREEN
I don't know what pybricks could or should do differently since theoretically each sensor is probably a bit different, especially when considering the different ways it may be installed.

Anyway, I point this out to start the conversation. Maybe there is something pybricks could do to make it easier for detecting colors.

2. Color class type hints (similar to this post). Like I said, I have only been programming python for a couple of years. Which means I am reading a lot of tutorials. My big kick lately has been to be a lot more careful about using type hints and mypy.
   
   The code still runs, but it looks like there are errors when there aren't. Turning mypy off "fixes" this, but something tells me it can be improved. I'd consider this very low priority, but perhaps something to start thinking about.

[dlech]

maybe this is kind of a python thing

Yeah, Python is dynamic. 😄

Is there a good reason to use one method over the other that I may not be thinking about?

Basically, the only difference is that modifying the Color dict makes a two-way association with the name and the color value. I.e. try print(my_dark_green) vs. print(Color.MY_DARK_BLUE ).

I wonder if there should be another set of built in colors.

I've thought the same before. But never made an official feature request. Feel free to start one.

2. but it looks like there are errors when there aren't.

You could try the dict syntax instead. Color["SENSOR_YELLOW"] to see if that makes the linter happy.

[MrGibbage]

I just remembered that I forgot to reply to this. I tried using the dict syntax, and it did not make the linter any happier.

I will submit a feature request for the sensor colors.

[dlech]

I tried using the dict syntax, and it did not make the linter any happier.

We can probably fix this by adding def __class_getitem__(self, name: str) -> Color: ... or a metaclass to the type hints for Color (feel free to open an issue or make a pull request for that).

https://github.com/pybricks/pybricks-api/blob/2a982641447acbb06bd9cdb96e798f1b6f58c734/src/pybricks/parameters.py#L74

[MrGibbage]

Submitted #2109