Skip to content

The colorsys library does not follow cpython implementation #28

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

Closed
Chimrod opened this issue May 8, 2023 · 3 comments · Fixed by #30
Closed

The colorsys library does not follow cpython implementation #28

Chimrod opened this issue May 8, 2023 · 3 comments · Fixed by #30

Comments

@Chimrod
Copy link

Chimrod commented May 8, 2023

Hello,

I’m openning this issue because I didn’t found any mention of this in the documentation in the page implementation-notes from the library.

There is an existing library colorsys in cpython with the methods named hls_to_rgb or hsv_to_rgb and the developper might be tempted to believe that the code running in cpython could run in circuitpython in the same way.

This is wrong : the functions in cpython returns a float with a range from 0…1 where circuitPython return an int between 0…255.

Could you at least add a caveat in the documentation in order to rise the attention on this point ?

@FoamyGuy
Copy link
Contributor

FoamyGuy commented May 8, 2023

Thanks for raising the issue.

Looking back at the original commits for this library it looks like it was originally taken from the cpython implementation and then with the next commit changed to 0-255 ints:

It's unclear why that change was made, perhaps the original authors intended use needed the 0-255 int colors.

Adding to the confusion the documented return types show float when in actuality the current version returns ints as noted. https://docs.circuitpython.org/projects/colorsys/en/latest/api.html#colorsys.hls_to_rgb

Instead of adding a warning in the implementation notes, I'd be in favor of changing the functionality to match cpython and return floats instead. If application code needs int colors they could convert it outside of this module so that we can keep compatibility to a maximum where possible.

@Chimrod
Copy link
Author

Chimrod commented May 8, 2023

Hello, I didn’t propose a change in the library itself because it would break the existing code using it. ( I can’t resist to insert an xkcd reference here )

However, this is my personnal point of view, and if you are willing to change the behavior itself, please do :)

@FoamyGuy
Copy link
Contributor

FoamyGuy commented May 8, 2023

A classic :)

That is true, and I'll definitely get input from the wider team.

I had a look around for usages in the most prominent places as well. In all of the Learn Guide repo, as well as all Library examples within the bundle there are two usages of colorsys but both appear to be within the context of CPython rather than CircuitPython:

Those were the only two I found in those places searching on "colorsys" string.

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 a pull request may close this issue.

2 participants