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

Don't convert JSON arrays to strings #4480

Merged
merged 2 commits into from Apr 27, 2018

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Apr 27, 2018

Replacement of #4469

(I accidentally submitted a PR against my master branch)

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Apr 27, 2018

@jasongrout This one just reuses the extract function directly above it to do the deep-copying. I think this is how it worked originally, then that JSONExt.isArray snuck in for some reason.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

ah, good catch! I'd collapse those two lines into one (we don't need that item variable anymore), and I think we have a winner!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Apr 27, 2018

Yep! So "convert multi-line strings to strings" sound like it probably serves a purpose... Is this meant to handle streams of text?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

It's meant to handle this case in the spec where long strings are broken by lines: https://nbformat.readthedocs.io/en/latest/format_description.html#display-data (i.e., the multiline string lines, see

type MultilineString = string | string[];
,
[key: string]: MultilineString | JSONObject;

I guess the real question here is: have those multiline strings been taken care of at this point in the code. I think so, and in fact it looks like things have been JSON.parsed as well. However, the typing indicates that it is still expecting something that might contain those multiline strings.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

I think we need to be more careful about distinguishing between the on-disk format data structures (which do include the multiline string type), and message information (which doesn't have this multiline string array).

This looks good. Thanks!

@jasongrout jasongrout merged commit 01f7b43 into jupyterlab:master Apr 27, 2018
2 checks passed
@gnestor gnestor deleted the output-json-array branch Apr 28, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Jun 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants