Skip to content
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.

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

full_figure_for_development #2737

Merged
merged 7 commits into from
Sep 2, 2020
Merged

full_figure_for_development #2737

merged 7 commits into from
Sep 2, 2020

Conversation

nicolaskruchten
Copy link
Contributor

Addresses the non-error portion of #1967

@jonmmease I'm not sure how to best test this or if such a trivial function is worth a test... ?

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Looks good, with one comment. As for testing, I think it would be worth adding a simple test under tests/test_optional (where kaleido is already installed for CI).

Just something simple that actually invokes kaleido and checks some expected default value.

if scope is None:
raise ValueError(
"""
Image export using the "kaleido" engine requires the kaleido package,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably refer to full_figure_for_development instead of Image export here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! done

@nicolaskruchten
Copy link
Contributor Author

OK added a test and some docs!

@nicolaskruchten
Copy link
Contributor Author

OK, all done! I had to add .decode("utf-8") to get bytes into strings for the Python 3.5 optional tests to pass ... normal? that'll work on 2.7 also right?

@nicolaskruchten
Copy link
Contributor Author

also I added a doc page... feedback welcome :) @emmanuelle if you want to take a look that'd be cool too :)

@jonmmease
Copy link
Contributor

OK, all done! I had to add .decode("utf-8") to get bytes into strings for the Python 3.5 optional tests to pass ... normal? that'll work on 2.7 also right?

Not sure about the Python 3.5 thing specifically, but decoding explicitly to utf-8 is the right thing to do.

@nicolaskruchten nicolaskruchten merged commit 4065a65 into master Sep 2, 2020
@archmoj archmoj deleted the full_fig branch November 23, 2021 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants