Andrew/fix encoder #406
Andrew/fix encoder #406
Conversation
src/py/kaleido/kaleido.py
Outdated
| try: | ||
| from choreographer import channels | ||
| from plotly.utils import PlotlyJSONEncoder | ||
|
|
||
| channels.register_custom_encoder(PlotlyJSONEncoder) | ||
| _logger.debug("Successfully registered PlotlyJSONEncoder.") | ||
| except ImportError: | ||
| _logger.debug("Couldn't import plotly- skipping.") |
There was a problem hiding this comment.
Separate the choreographer import into a separate try/except so that the logger message can be more accurate
There was a problem hiding this comment.
I changed the log to include the error, I kept the imports together since we can bail out of importing channels if there is no Plotly. Let me know if that's cool.
src/py/CHANGELOG.txt
Outdated
| @@ -1,3 +1,6 @@ | |||
| v1.2.0 | |||
| - Add plotly dependency: use its encoder instead of our custom. | |||
There was a problem hiding this comment.
update changelog entry (Plotly not a dependency)
You can see the start of a new concept for integration tests where I use pickled figures instead of JSON mocks to provoke JSON encoding errors. That's where this type of test belongs, I believe, because our unit tests shouldn't lock down the choreographer or _plotly_utils behavior. Happy to chat about it or write a README if you prefer. Don't want to add that feature to this pr though, so I just created the structure from it and in the next PR I'll execute the tests, since that is what I'm working on anyway. |
emilykl
left a comment
emilykl
left a comment
There was a problem hiding this comment.
Should have a unit test for this behavior, otherwise LGTM 🚀
Depends on https://github.com/plotly/choreographer/pull/249/filesKaleido will now see if plotly is available to use its JSONEncoder if so.