Make offline.plot/iplot accept a plot config dict #714
Conversation
|
@kernc taking a look right now |
| config['showLink'] = show_link | ||
| config['linkText'] = link_text | ||
| config = dict(config) if config else {} | ||
| config.setdefault('showLink', show_link) |
There was a problem hiding this comment.
Why not just write config['showLink] = show_link? It's clearer to me.
Same goes with config.setdefault('linkText', link_text)
There was a problem hiding this comment.
The reason for dict.setdefault is that it doesn't set the value if it is already set. This gives user the option to construct the whole view configuration dict beforehand, not needing to worry about having showLink overwritten or needing to pass it separately as show_link. I'd rather go with:
if 'linkText' not in config:
config['linkText'] = link_textbut I think .setdefault() is pretty standard. Do you insist? The behavior of not overriding the option if it's already set is documented.
There was a problem hiding this comment.
Nope, don't insist. I get the reasoning now.
| config['showLink'] = show_link | ||
| config['linkText'] = link_text | ||
| config = dict(config) if config else {} | ||
| config.setdefault('showLink', show_link) |
| self.assertTrue(data_json in html) # data is in there | ||
| self.assertTrue(layout_json in html) # so is layout | ||
| self.assertTrue(PLOTLYJS in html) # and the source code | ||
| self.assertIn('Plotly.newPlot', html) # plot command is in there |
There was a problem hiding this comment.
Fantastic edit! Simplifies the function of the test with the assertIn method
| # and it's an <html> doc | ||
| self.assertTrue(html.startswith('<html>') and html.endswith('</html>')) | ||
|
|
||
| def test_including_plotlyjs(self): | ||
| html = self._read_html(plotly.offline.plot(fig, include_plotlyjs=False, | ||
| auto_open=False)) | ||
| self.assertTrue(PLOTLYJS not in html) | ||
| self.assertNotIn(PLOTLYJS, html) |
|
@kernc I've reviewed and just have a couple suggestions for improving clarity of code. It's a 💃 from me after that. |
The checked strings were never in that file URL. The file needs to be read.
The benefit is in the more informative context when the tests fail.
|
💃 from me. (Reminder for myself to update CHANGELOG 🐱 ) |
Make
plotly.offline.plot()(iplot()) accept aconfigdictionary so the various configuration options can be passed. Includes unit tests.Addresses https://community.plot.ly/t/removing-hover-toolbar-and-other-configuration-options-with-the-python-api/374.
Also took liberty to slightly improve surrounding
offlinetests.If you find the proposed feature something acceptable, I'd be happy to also amend the docs.