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

properly treat JSON data #847

Merged
merged 1 commit into from Aug 16, 2018

Conversation

Projects
None yet
2 participants
@akhmerov
Copy link
Contributor

akhmerov commented Jul 26, 2018

closes #846

Still TODO: tests.

I would additionally appreciate an opinion whether this fix is clean enough.

@@ -73,6 +75,10 @@ def preprocess_cell(self, cell, resources, cell_index):
if mime_type in out.data:
data = out.data[mime_type]

# If data was JSON, it will become a NotebookNode at this point.
if isinstance(data, NotebookNode):
data = json.dumps(data)

This comment has been minimized.

@minrk

minrk Aug 10, 2018

Member

This shouldn't actually be turning it to text, I think. That would do incorrect things for --to notebook, for example. I think ExtractOutputPreprocessor needs to be taught how to handle when this is a non-string (i.e. json serialize as part of writing to a file), rather than casting it to a string here.

Test ideas:

  • convert to notebook and verify that json outputs are preserved as objects
  • extract output with json
  • make sure to include a variety of json outputs which can be dicts, lists, scalars, etc., not just dicts.

This comment has been minimized.

@akhmerov

akhmerov Aug 11, 2018

Author Contributor

convert to notebook and verify that json outputs are preserved as objects

I believe this was already the case: we're dealing with a preprocessor that doesn't modify the cell outputs.

I have added a test to verify that it is indeed the case.

@akhmerov akhmerov force-pushed the akhmerov:bugfix/issue-846 branch 2 times, most recently from 5f8295d to 51678c2 Aug 10, 2018

@akhmerov

This comment has been minimized.

Copy link
Contributor Author

akhmerov commented Aug 10, 2018

@minrk I think this is good now. Thanks for the suggestion to check multiple data types of JSON, this was very useful.

On a side-note: is a string scalar value allowed by nbformat? In the spec itself I see nothing against it, but IPython.display.JSON only allows to represent container and not scalar data.

@akhmerov

This comment has been minimized.

Copy link
Contributor Author

akhmerov commented Aug 10, 2018

OK, tests from other preprocessors are failing due to the modification of the example notebook. What's the better strategy here? Update other tests or separate the test case for this preprocessor?

@akhmerov akhmerov force-pushed the akhmerov:bugfix/issue-846 branch from 51678c2 to 720509b Aug 11, 2018

@akhmerov

This comment has been minimized.

Copy link
Contributor Author

akhmerov commented Aug 11, 2018

@minrk I think this is ready. Travis failure on python 2.7 seems unrelated, and a glitch.

I didn't see the full logs; will investigate.

@akhmerov akhmerov force-pushed the akhmerov:bugfix/issue-846 branch 2 times, most recently from 1b93428 to a7beef2 Aug 13, 2018

@akhmerov akhmerov force-pushed the akhmerov:bugfix/issue-846 branch from a7beef2 to 3d1a76f Aug 13, 2018

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Aug 16, 2018

is a string scalar value allowed by nbformat?

I believe it should be. Initially, JSON had to be a dict, but now I believe any JSON type should work.

@minrk minrk added this to the 5.4 milestone Aug 16, 2018

@minrk minrk merged commit 61f02a0 into jupyter:master Aug 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment