Fix ColorScale presentation when specified as string #1089
Conversation
_plotly_utils/basevalidators.py
Outdated
| if v is None: | ||
| return None | ||
| elif isinstance(v, str): |
There was a problem hiding this comment.
Please use isinstance(v, string_types) for Python 2 compatibility
|
|
||
| # Presented as a string | ||
| self.assertEqual(self.scatter.marker.colorscale, | ||
| "Viridis") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Added. I think that's everything, right?
Added tests checking that a colorscale specified as a string is returned correctly. Previously it had been returned as a tuple of 1-tuples. e.g. "Viridis" -> (('V',), ('i',), ('r',), ('i',), ('d',), ('i',), ('s',)). Catches plotly#1087.
ivirshup
force-pushed
the
ColorScaleValidator_string-handling
branch
from
233f26d
to
5aff631
Compare
|
Looks great. Thanks for the contribution @ivirshup! |
Fixes #1087.
I kinda guessed with where I put the test, let me know if there's a better place for it.