jupyter-server / jupyter_ydoc

Jupyter document structures for collaborative editing using Yjs/pycrdt
https://jupyter-ydoc.readthedocs.io
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

Removes YMap for attachements #77

Closed fcollonval closed 2 years ago

fcollonval commented 2 years ago

This is similar to https://github.com/jupyter-server/jupyter_ydoc/pull/50

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

davidbrochart commented 2 years ago

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

fcollonval commented 2 years ago

Yes

fcollonval commented 2 years ago

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

fcollonval commented 2 years ago

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

davidbrochart commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

davidbrochart commented 2 years ago

I made both create_ycell and get_cell symetrical in https://github.com/jupyter-server/jupyter_ydoc/pull/77/commits/8a8c7385ed684c17429e3f4509b993e574237f03: we remove attachments if it's an empty dict.

hbcarlos commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks!

fcollonval commented 2 years ago

@meeseeksdev please backport to 0.2.x

davidbrochart commented 2 years ago

@meeseeksdev please backport to 0.2.x

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

fcollonval commented 2 years ago

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

davidbrochart commented 2 years ago

@meeseeksdev please backport to 0.2.x

fcollonval commented 2 years ago

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

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

davidbrochart commented 2 years ago

@MeeseeksDev Hello

lumberbot-app[bot] commented 2 years ago

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

davidbrochart commented 2 years ago

@meeseeksdev please backport to 0.2.x

davidbrochart commented 2 years ago

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

Seems to work now!