Test fixes 2 #994
Test fixes 2 #994
Conversation
…h retrying decorator to fix remaining tests
|
@jmmease @chriddyp For the streaming bit, I tweaked a few things in the code and would like some feedback. The main problem was that the validation path for Stream.write() To fix this I decided to just let the code do Jon's tight and immediate validation. |
| @@ -31,6 +31,7 @@ | |||
| from plotly.api import v1, v2 | |||
| from plotly.basedatatypes import BaseTraceType | |||
| from plotly.plotly import chunked_requests | |||
| from plotly.graph_objs import Scatter | |||
There was a problem hiding this comment.
does it create an issue to create a dependancy of graph_objs for plotly.py?
There was a problem hiding this comment.
I don't think so. There shouldn't be an existing dependency the other way (i.e. plotly.graph_objs -> plotly.plotly)
There was a problem hiding this comment.
That's good. I only brought it up because another engineer advised against doing that i.e. sent me an blogpost saying it could be a problem
| @@ -588,7 +588,7 @@ def open(self): | |||
| streaming_specs = self.get_streaming_specs() | |||
| self._stream = chunked_requests.Stream(**streaming_specs) | |||
|
|
|||
| def write(self, trace, layout=None, validate=True, | |||
There was a problem hiding this comment.
removed validate so that users will not be able to use the internal validation
|
|
||
| stream_object = dict() | ||
| stream_object.update(trace) | ||
| if 'type' not in stream_object: | ||
| # tests if Scatter contains invalid kwargs | ||
| dummy_obj = copy.deepcopy(Scatter(**stream_object)) |
There was a problem hiding this comment.
Here I am making a deepcopy of the trace being coerced into a Scatter from the original dict. The point is that an error will pop up if there is one, and stream_object can continue down the code being untouched and do its thing.
Do you think this is a good idea? Is it better to have the actual object the user sent raise the error?
|
@jmmease all the tests are passing in py3.6. Are you going to merge my PR to #942? |
|
Awesome!!! Sure I'll merge it on in |
|
@Kully Also, I'm not going to be active on this for a few weeks so why don't you go ahead and just work on the |
Continuation of #975 (comment)
Fixing Minor Tests for Ipyplotly Integration PR
cc. @jmmease @chriddyp