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

Removes YMap for attachements #77

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Conversation

fcollonval
Copy link
Member

This is similar to #50

To test the changes, make sure you remove the jupyter_ystore database.

@davidbrochart
Copy link
Collaborator

Just to clarify, is this to match what's in JupyterLab's master branch?

@fcollonval
Copy link
Member Author

Yes

@fcollonval
Copy link
Member Author

Otherwise for now if you open the examples/notebooks/OutputExamples.ipynb, it fails because the attachments is YMap not a JSON object.

@fcollonval
Copy link
Member Author

The error is a infinite recursive call within JSONExt.deepCopy because the YMap reference its parent.

@davidbrochart
Copy link
Collaborator

Thanks.
BTW, why do we enforce the "attachments" field when we create a raw or markdown cell (with {} when there are no attachments) and remove it when we get a cell (if there are are no attachments)?

@fcollonval
Copy link
Member Author

why do we enforce the "attachments" field when we create a raw or markdown cell (with {} when there are no attachments) and remove it when we get a cell (if there are are no attachments)?

No idea - but indeed it will be good to simply not set attachments in such case.

@davidbrochart
Copy link
Collaborator

This was done in #27. Maybe @hbcarlos knows?

@davidbrochart
Copy link
Collaborator

I made both create_ycell and get_cell symetrical in 8a8c738: we remove attachments if it's an empty dict.

@davidbrochart davidbrochart added the bug Something isn't working label Oct 13, 2022
@hbcarlos
Copy link
Contributor

hbcarlos commented Oct 13, 2022

Thanks.
BTW, why do we enforce the "attachments" field when we create a raw or markdown cell (with {} when there are no attachments) and remove it when we get a cell (if there are are no attachments)?

Because we need to initialize that attribute even when it is empty.

Why are we deleting it before saving it? I don't recall. Probably the nbformat doesn't allow empty attachments.

@davidbrochart
Copy link
Collaborator

Because we need to initialize that attribute even when it is empty.

Why? The documentation says:
"The attachments dictionary is an optional field and can be undefined or empty if the cell does not have any attachments."

@davidbrochart davidbrochart merged commit fb09c48 into jupyter-server:main Oct 25, 2022
@davidbrochart
Copy link
Collaborator

Thanks!

@fcollonval fcollonval deleted the patch-1 branch October 26, 2022 06:53
@fcollonval
Copy link
Member Author

@meeseeksdev please backport to 0.2.x

@davidbrochart
Copy link
Collaborator

@meeseeksdev please backport to 0.2.x

Oh, I didn't know we could do that here 😄

@fcollonval
Copy link
Member Author

Actually I don't have the right to do it. But you should be able.

@davidbrochart
Copy link
Collaborator

@meeseeksdev please backport to 0.2.x

@fcollonval
Copy link
Member Author

It seems the Meeseeksdev GitHub App is not installed on this repository 😢

You should be able to add it in the settings of the project.

@davidbrochart
Copy link
Collaborator

@meeseeksdev Hello

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 26, 2022

Helloooo @davidbrochart, I'm Mr. Meeseeks! Look at me!

@davidbrochart
Copy link
Collaborator

@meeseeksdev please backport to 0.2.x

@davidbrochart
Copy link
Collaborator

You should be able to add it in the settings of the project.

Seems to work now!

davidbrochart added a commit that referenced this pull request Oct 26, 2022
…-0.2.x

Backport PR #77 on branch 0.2.x (Removes YMap for attachements)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants