fixing the bug of update_ methods

[Mahdis-z]

resolves #2167

[jdamiba]

Test Plan:

Currently in production, the following code throws an error. On this feature branch however it works as expected.

fig = go.Figure()

fig.add_trace(go.Scatter(
     x=[0, 1, 2, 3, 4, 5, 6, 7, 8],
     y=[0, 1, 3, 2, 4, 3, 4, 6, 5]
 ))

fig.update_layout(
     showlegend=False,
     annotations=[
         dict(
             x=2,
             y=5,
             xref="x",
             yref="y",
             text="dict Text",
             showarrow=True,
             arrowhead=7,
             ax=0,
             ay=-40
         )
     ]
)

fig.update_annotations(text='changed')
fig.show()

Changes look good! 💃

[emmanuelle]

Thank you Mahdis and thank you Joe for the test idea. Since there is already a test class in test_update_objects/test_update_annotations.py (defining a figure which is then used in class methods for various tests), the best thing is probably to take it from this class and a new method called test_xxx (like test_annotation_attributes).

[jdamiba]

The tests added by @Mahdis-z added pass, which is awesome! The relevant pytest output:

packages/python/plotly/plotly/tests/test_core/test_update_objects/test_update_annotations.py::TestSelectForEachUpdateAnnotations::test_annotation_attributes PASSED [ 25%]

packages/python/plotly/plotly/tests/test_core/test_update_objects/test_update_annotations.py::TestSelectForEachUpdateAnnotations::test_image_attributes PASSED [ 33%] 

packages/python/plotly/plotly/tests/test_core/test_update_objects/test_update_annotations.py::TestSelectForEachUpdateAnnotations::test_shape_attributes PASSED [ 75%]

I only have one suggestion. When I try to run fig.update_shapes(text='blue') or fig.update_layout_images(text='hi') outside of the test environment I get an error:

ValueError: Invalid property specified for object of type plotly.graph_objs.layout.Shape: 'text'
ValueError: Invalid property specified for object of type plotly.graph_objs.layout.Image: 'text'

Therefore, wouldn't it be safer to update a property in the object (such as fillcolor for Shape and opacity for Image) rather than text? @emmanuelle Any thoughts?

[emmanuelle]

@jdamiba you mean using this branch but outside of pytest? This could also explain why the CI build using nosetests fails, but then I don't understand why pytest does not raise an error... Anyway yes in the test a property of the object should be used.

[emmanuelle]

Also, black is not happy (@Mahdis-z do you know how to use black? ping me otherwise)

[Mahdis-z]

Also, black is not happy (@Mahdis-z do you know how to use black? ping me otherwise)

@jdamiba @emmanuelle Thanks for the review and comments 👍 not sure what am I missing that black is still unhappy

[emmanuelle]

Did you run black on test_update_annotations.py (black test_update_annotations.py)? If yes it could be an inconsistency between black versions.

[jdamiba]

It is not obvious to me why the plotlyjs_dev_build CI job is failing.

The error is the one that this PR supposed to address:

======================================================================
ERROR: test_annotation_attributes (plotly.tests.test_core.test_update_objects.test_update_annotations.TestSelectForEachUpdateAnnotations)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/circleci/project/packages/python/plotly/plotly/tests/test_core/test_update_objects/test_update_annotations.py", line 223, in test_annotation_attributes
    self.fig.update_annotations(text="hi")
TypeError: update_annotations() missing 1 required positional argument: 'patch'

----------------------------------------------------------------------

@emma Any ideas why this particular container is failing but not any others?

[emmanuelle]

Not really. This build is using nosetests with a '!dev' option which I don't understand...

[nicolaskruchten]

Does that CI job use the latest release and not master or something?

[emmanuelle]

Does that CI job use the latest release and not master or something?

could be, but then it would have happened with previous bug fixes adding tests... Taking a look right now.

[nicolaskruchten]

So the tests as written I don't think are actually running updates on a figure which contains a shape/annotation/image, which is why .update_shapes(text='x') doesn't fail... In general I think the current tests are a bit too minimal: we should actually construct a minimal figure with a shape/annotation/image, run the update and assert that it worked :)

[emmanuelle]

maybe this would even fix the nosetests problem ?

[emmanuelle]

@Mahdis-z for example do

self.fig.add_annotation(dict(x=1, y=1))
self.fig.update_annotations(text="hi")

so that there is already an annotation

[emmanuelle]

So I'm really having a hard time understanding what's going on here. I ran a few tests printing stuff in #2216 and it seems that the function signature is indeed the old one (with patch a positional arg) while the path is correct, the plotly version is the dev one...

What I suggest is

• @Mahdis-z , you update the tests so that the figure does have a shape, image, annotation etc when it's tested
• I continue to test maybe with pytest instead of nose

[emmanuelle]

I think you wanted text="this text" here. (CI fails because test is not valid)

[Mahdis-z]

ohh yep!

[nicolaskruchten]

OK I think I know why this is happening... the files you've edited are actually the output of the code generation, so you also need to edit the codegen system so that the next time codegen is run, the changes are applied.

Basically you need to add the =None here: https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/codegen/figure.py#L476

[emmanuelle]

ok, so when the codegen is changed, the procedure is normally to

• update one of the codegen files, add it to staged changes
• run locally the codegen on your machine (python setup.py codegen I think)
• add files modified by the codegen (graph_objs/figure.py) to the stages changes
• commit and push

But here you modified manually the graph_objs/figure.py so it does the same I think. 💃 once it is green!

[nicolaskruchten]

I'm trying the codegen now to make sure it matches the mods here.

[nicolaskruchten]

Looks clean! Still waiting for 💚

[Mahdis-z]

ok, so when the codegen is changed, the procedure is normally to

• update one of the codegen files, add it to staged changes
• run locally the codegen on your machine (python setup.py codegen I think)
• add files modified by the codegen (graph_objs/figure.py) to the stages changes
• commit and push

But here you modified manually the graph_objs/figure.py so it does the same I think. 💃 once it is green!

Perfect! thx