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.

Sign up for GitHub

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

Jump to bottom

Fix ColorScale presentation when specified as string #1089

Merged
merged 3 commits into from
Aug 6, 2018
Merged

Fix ColorScale presentation when specified as string #1089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66b6782
Added tests for colorscales specified as a string
ivirshup Aug 2, 2018
5aff631
Fix presentation of string colorscales
ivirshup Aug 2, 2018
f793faa
Added support for Cividis to ColorScaleValidator
ivirshup Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions _plotly_utils/basevalidators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,7 @@ class ColorscaleValidator(BaseValidator):
named_colorscales = [
'Greys', 'YlGnBu', 'Greens', 'YlOrRd', 'Bluered', 'RdBu', 'Reds',
'Blues', 'Picnic', 'Rainbow', 'Portland', 'Jet', 'Hot', 'Blackbody',
'Earth', 'Electric', 'Viridis'
'Earth', 'Electric', 'Viridis', 'Cividis'
]

def __init__(self, plotly_name, parent_name, **kwargs):
Expand All @@ -1229,7 +1229,7 @@ def description(self):
- One of the following named colorscales:
['Greys', 'YlGnBu', 'Greens', 'YlOrRd', 'Bluered', 'RdBu',
'Reds', 'Blues', 'Picnic', 'Rainbow', 'Portland', 'Jet',
'Hot', 'Blackbody', 'Earth', 'Electric', 'Viridis']
'Hot', 'Blackbody', 'Earth', 'Electric', 'Viridis', 'Cividis]
""".format(plotly_name=self.plotly_name)

return desc
Expand Down Expand Up @@ -1274,9 +1274,11 @@ def validate_coerce(self, v):
return v

def present(self, v):
# Return tuple of tuples so that colorscale is immutable
# Return-type must be immutable
if v is None:
return None
elif isinstance(v, string_types):
return v
else:
return tuple([tuple(e) for e in v])

Expand Down
5 changes: 4 additions & 1 deletion _plotly_utils/tests/validators/test_colorscale_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ def validator():


@pytest.fixture(params=['Greys', 'YlGnBu', 'Greens', 'YlOrRd', 'Bluered', 'RdBu', 'Reds', 'Blues',
'Picnic', 'Rainbow', 'Portland', 'Jet', 'Hot', 'Blackbody', 'Earth', 'Electric', 'Viridis'])
'Picnic', 'Rainbow', 'Portland', 'Jet', 'Hot', 'Blackbody', 'Earth', 'Electric',
'Viridis', 'Cividis'])
def named_colorscale(request):
return request.param

Expand All @@ -26,6 +27,8 @@ def test_acceptance_named(named_colorscale, validator: ColorscaleValidator):
# Uppercase
assert (validator.validate_coerce(named_colorscale.upper()) ==
named_colorscale.upper())

assert validator.present(named_colorscale) == named_colorscale

# ### Acceptance as array ###
@pytest.mark.parametrize('val', [
Expand Down
10 changes: 10 additions & 0 deletions plotly/tests/test_core/test_graph_objs/test_properties_validated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ def test_present_colorscale(self):
# Presented as tuple of tuples
self.assertEqual(self.scatter.marker.colorscale,
((0, 'red'), (1, 'green')))

def test_present_colorscale_str(self):
self.assertIsNone(self.scatter.marker.colorscale)

# Assign string
self.scatter.marker.colorscale = "Viridis"

# Presented as a string
self.assertEqual(self.scatter.marker.colorscale,
"Viridis")
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to this test, we should update the test_acceptance_named validator test in _plotly_utils/tests/validators/test_colorscale_validator.py

It should work a bit more like the test_acceptance_array test in that same file (the second one, it's a mistake that there are two with the same name). Where the coerced value is held onto and then run through the validators present method.

If it's not obvious how this should work don't spend much time on it, I need to do some cleanup in there soon anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone ahead and updated that test. Would it be worthwhile to set the named_colorscale fixtures possible values to be set either by ColorScaleValidator.named_colorscales or some schema to be sure it's up to date?

Also, should Cividis be on those lists?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd like to keep the explicit list of named color scales in the tests. It is duplicated with the list in ColorScaleValidator.named_colorscales, but if we ever try to pull the validator's named colorscales list out of the schema I'd like to have this explicit list in the tests

Yes, good call on Cividis. Could you add that to both the test fixture and the named_colorscales list? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. I think that's everything, right?



class TestPropertyIterContains(TestCase):
Expand Down