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

Fix ColorScale presentation when specified as string #1089

Merged

Conversation

ivirshup
Copy link
Contributor

@ivirshup ivirshup commented Aug 2, 2018

Fixes #1087.

I kinda guessed with where I put the test, let me know if there's a better place for it.

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution and for helping push version 3 forward @ivirshup !

One small change needed, and a testing suggestion below.

if v is None:
return None
elif isinstance(v, str):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use isinstance(v, string_types) for Python 2 compatibility


# 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?

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 ivirshup force-pushed the ColorScaleValidator_string-handling branch from 233f26d to 5aff631 Compare August 3, 2018 04:28
@jonmmease jonmmease merged commit 313129b into plotly:master Aug 6, 2018
@jonmmease
Copy link
Contributor

Looks great. Thanks for the contribution @ivirshup!

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 this pull request may close these issues.

2 participants