-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Improve color provider #1055
Improve color provider #1055
Conversation
faker/providers/color/color.py
Outdated
for color_name, color_attrs in self.colormap.items(): | ||
lower_bounds = color_attrs['lower_bounds'] | ||
s_min = lower_bounds[0][0] | ||
s_max = lower_bounds[len(lower_bounds) - 1][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this equivalent to s_max = lower_bounds[-1][0]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I wonder why the author of randomcolor-py
did not just use [-1]
for this one. I will fix this, and I am sorry I did not notice.
faker/providers/color/color.py
Outdated
if color['hue_range'][0] <= hue <= color['hue_range'][1]: | ||
return self.colormap[color_name] | ||
else: | ||
raise ValueError('Value of hue is invalid.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also show the value itself in the exception message? Something like 'Value of hue `%s` is invalid.' % hue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only happen if something is wrong with COLOR_MAP
itself, but sure.
faker/providers/color/color.py
Outdated
v1 = int(v1) | ||
v2 = int(v2) | ||
except (ValueError, TypeError): | ||
return [0, 360] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'd want it to silently fallback to defaults. I'd rather have it throw an exception if the user is passing invalid values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I will change this behavior.
The flake8 errors are related to PR #1058. |
Looks Good! Thank you so much! |
What does this changes
This will add a new color provider method
color()
that allows specifying a hue, a luminosity, and a color format to generate an matching color.What was wrong
There is currently no support for such features.
Notes
Personally, I prefer to return tuples of HSV, HSL, and RGB values instead of the
color_format(X, Y, Z)
string notation or any string for that matter, so that there is no need for users to parse the string for the values. I followed the current wayrgb_color()
andrgb_css_color()
do this, but I am not a fan of those methods.I also think that it would be better to provide more convenient methods and update old ones, so users will not need to remember the different
color_format
values while still being able to just specify 'blue' and 'dark' to get a color of that type.Fixes #104