Skip to content
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

Bugfix: Use color.lower() for _color_dict keys #2459

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

psobolewskiPhD
Copy link
Contributor

@psobolewskiPhD psobolewskiPhD commented Mar 6, 2023

The _string_to_rgb helper function uses color.lower() to raise a ValueError, because the names are all lower case in _color_dict. However, in the next line, it still uses color as the key to the dict, which means that if the string is upper case, the first logic passes, but then a KeyError happens. This was first reported here in napari:
napari/napari#5599 (comment)

So this PR simply makes sure to use color.lower() as the key to the _color_dict dictionary.

All tests pass for me locally with this and the napari issue is fixed.

Copy link
Collaborator

@brisvag brisvag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy peasy!

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks. This logic makes me wonder if we should have some warning to tell the user that they are using a different name...but maybe this is fine as a nice convenience.

@psobolewskiPhD
Copy link
Contributor Author

I think adding a warning makes some sense, just as an FYI, even though the code then uses .lower(). Want me to add that?

@djhoese
Copy link
Member

djhoese commented Mar 7, 2023

I'm thinking maybe no warning. Then at a user-level people are fine to say "Red" or "red". Maybe that's OK...but then maybe the docstring should mention this? Ugh it is such a small decision that I'm sure will bite us in the future.

@brisvag
Copy link
Collaborator

brisvag commented Mar 8, 2023

IMHO waring is unnecessary. Worst that can happen (last famous words...) is the user somehow checking 'Red' == 'red' and failing, which is a pretty easy solution.

@djhoese djhoese merged commit 2d583f4 into vispy:main Mar 8, 2023
@djhoese
Copy link
Member

djhoese commented Mar 8, 2023

I think this is good as-is. Thanks @psobolewskiPhD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants