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

Improve Python API #122

Merged
merged 10 commits into from
Jan 14, 2023
Merged

Conversation

davidbrochart
Copy link
Collaborator

Closes #65.

@davidbrochart
Copy link
Collaborator Author

@fcollonval I believe commit b7ab80a addresses your concerns in #65. See jupyterlab/jupyter-collaboration#72 for an example of the change required in the callback.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

It seems to be a good improvement indeed. You probably need to update the signature of YBaseDoc.observe too.

@davidbrochart
Copy link
Collaborator Author

You probably need to update the signature of YBaseDoc.observe too.

Yes, thanks.

@davidbrochart davidbrochart changed the title Improve YNotebook Python API Improve Python API Jan 10, 2023
@davidbrochart davidbrochart marked this pull request as ready for review January 10, 2023 14:25
@davidbrochart
Copy link
Collaborator Author

Commit a75ea4e partially addresses #123 by adding YUTF8 (same as YFile) and YBytes. YBytes doesn't support changing individual bytes, only the whole content (see y-crdt/ypy#108 (comment)).

@@ -153,9 +155,9 @@ def unobserve(self) -> None:
self._subscriptions = {}


class YFile(YBaseDoc):
class YUTF8(YBaseDoc):
Copy link
Member

Choose a reason for hiding this comment

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

Should we use something more generic like YText?

the idea is a bit like for YBytes to let the door open to other encoding. And also it is probably more easy to grasp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we use something more generic like YText?

Maybe, although YText already exists in y-py, but you're right, if they don't call it YUTF8 there then I guess there is no reason that we do it here.

the idea is a bit like for YBytes to let the door open to other encoding. And also it is probably more easy to grasp.

Actually I'd like to rename YBytes to YBlob, which reflects the idea that it's a whole chunk of data on which we cannot collaborate on an element basis. Also, it can store anything, not only bytes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

04a7f74 renames YBytes to YBlob. Actually, it really stores bytes, which is fine since blob stands for "binary large object".

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing the existing YText in y-py. So maybe YText here is not the most appropriate as it can generate confusion. I'm a bit short of idea - I thought about YStr or YString but i'm half convinced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Traitlets has Unicode. It's similar to the previous YUTF8, but maybe more inline with the rest of the Jupyter ecosystem?

@davidbrochart davidbrochart merged commit db30bfa into jupyter-server:main Jan 14, 2023
@davidbrochart davidbrochart deleted the notebook branch January 14, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve observe API
2 participants