Skip to content

Fixes #414 #416

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 30, 2016
Merged

Fixes #414 #416

merged 4 commits into from
Dec 30, 2016

Conversation

tarzzz
Copy link
Contributor

@tarzzz tarzzz commented Mar 2, 2016 •
edited
Loading

Fixes #414 and #384

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 2, 2016

@cldougl Please review. I'll add some test cases as well..

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 3, 2016

Yep, it works for the example. Added tests as well.. :)

@tarzzz
Copy link
Contributor Author

tarzzz commented Mar 3, 2016

@yankev : Getting 502, Bad gateway error on un-related tests (unrelated to this PR). :/

Update: It is passing now..

@tarzzz tarzzz mentioned this pull request Mar 8, 2016
Closed
@theengineear
Copy link
Contributor

@tarzzz any update on this? seems like it'd be helpful :)

@tarzzz
Copy link
Contributor Author

tarzzz commented Dec 29, 2016

@theengineear It is good to go in (I almost forgot I had made this PR) .. a quick 👀 from you or @cldougl before I merge it in .. 😺

theengineear
theengineear reviewed Dec 29, 2016
View reviewed changes
plotly/matplotlylib/mplexporter/exporter.py
if not (child is legend.get_children()[-1]
and child.get_text() == 'None'):
if (child is not legend.get_children()[-1]
and child.get_text() != 'None'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment on this change? It seems unrelated?

Copy link
Contributor Author

@tarzzz tarzzz Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@theengineear

This fixes #384 ..
I can made it as a different PR, or we can use this PR to kill two birds with a single stone.. 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, gotcha. 👍

theengineear
theengineear reviewed Dec 29, 2016
View reviewed changes
plotly/utils.py
if isinstance(obj, decimal.Decimal):
return float(obj)
else:
raise NotEncodable
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@theengineear
Copy link
Contributor

@tarzzz, the default schema you have is likely quite out of date at this point. Would you mind:

  1. removing your commit for the default schema change
  2. addressing the change to the matplotlylib code (and possibly reverting that?)
  3. rebasing on the most current master
  4. updating the default schema to the newest version?

I'm also happy to do this, but I am just a little unsure about the motivation for (2).

@tarzzz
Copy link
Contributor Author

tarzzz commented Dec 30, 2016

@theengineear : Rebased with master, and updated the plot-schema..

@theengineear
Copy link
Contributor

Let's 💃 !

@tarzzz tarzzz merged commit ab67020 into master Dec 30, 2016
@tarzzz tarzzz deleted the issue-414 branch December 30, 2016 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theengineear theengineear theengineear left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tarzzz @theengineear