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

Fixed latex export for svg data in python 3 #985

Merged
merged 3 commits into from
Apr 14, 2019

Conversation

MSeal
Copy link
Contributor

@MSeal MSeal commented Apr 7, 2019

Fixes #981

Verified that the rendered latex output matches python 2 output visually. This will probably fix other python exports outside of latex svg given the code path involved in the fix.

@MSeal MSeal requested a review from mpacer April 7, 2019 21:43
@MSeal MSeal added this to the 5.5 milestone Apr 7, 2019
not isinstance(data, text_type)
or mime_type == 'application/json'
):
if mime_type == 'application/json':
Copy link
Member

Choose a reason for hiding this comment

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

This is too strict and will break some data types; compare with even a partial listing here: https://jupyter.readthedocs.io/en/latest/reference/mimetype.html

Also this change invalidates the follow-up comment, please keep them in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point on the strictness.

@akhmerov
Copy link
Member

In the original implementation I've relied on the nbformat spec that guaranteed that json data is parsed directly and not serialized.

I don't actually understand why that bit requires changing to support svg: IIUC svg should be of text-type and shouldn't be treated as json by that code.

@MSeal
Copy link
Contributor Author

MSeal commented Apr 14, 2019

If you only add the svg test I added you'll see if fails for python 3 in the same way the original issue describes. The data object coming back from out.data[mime_type] is definitely bytes and adding data = data.decode('utf-8'), or even the original data = json.dumps(data), generically to all byte data before any other processing seemed wrong.

I'll look into the test case I added and see if there's a less strict way to solve the issue.

@akhmerov
Copy link
Member

At the very least lists/dicts should be detected as definitely json

@MSeal
Copy link
Contributor Author

MSeal commented Apr 14, 2019

I looked into the failing pattern, it's when the mimetype is application/pdf that it fails in the test case originally.

I changed the implementation to mirror the original behavior more closely, with just logically moving the base64 data type checks in front of the json / text checks. Can you take a look again @akhmerov for any issue you can see?

@MSeal
Copy link
Contributor Author

MSeal commented Apr 14, 2019

Thanks for the review!

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.

Failure converting to LaTeX
2 participants