-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use pycrdt instead of y-py #194
Conversation
37d82aa
to
0404622
Compare
6ece654
to
a66b51d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidbrochart for working on this. I was about to ask if the two libraries could be use transparently. But I'm seeing that it is definitely not possible.
I have a couple of comments.
jupyter_ydoc/yblob.py
Outdated
@@ -55,7 +56,7 @@ def get(self) -> bytes: | |||
:return: Document's content. | |||
:rtype: bytes | |||
""" | |||
return base64.b64decode(self._ysource.get("base64", "").encode()) | |||
return base64.b64decode(self._ysource["base64"].encode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does pycrdt add dynamically a requested key - what would be a bad idea. If not we cannot be sure base64
exists on _ysource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, pycrdt doesn't create a key on read, and yes you're right I should put back the previous behavior. Thanks, I don't know why I changed it.
It may be possible to create a wrapper around Ypy so that it has the same API as pycrdt, but at the same time pycrdt could evolve in a different direction in the future, so I'm not sure I should invest time in Ypy if in the end it's not possible for them to converge on the same API. |
Linking here the discussion on jupyter-server/team-compass#55, I guess we first need to have an understanding and agreement on y-py/pycrdt before merging this PR. |
Are there any tangible user-benefits (for end users) for switching to pycrdt? It should be front of mind that there's a cost to switching libraries, and not just the initial development cycles.
Adding extra languages, binaries, tools and workflows can be a burden on maintainers and increase complexity and difficulty for new contributors. We should have a COMPLETE understanding of the impacts before moving forward. The cost of increased complexity, added dependencies and workflows should not be accepted unless end users are going to see substantial benefits... |
I'm obviously doing this for the good of the project. Ypy is close to being unmaintained, part of the reason being its complexity. With pycrdt I want to make my life easier in order to improve collaborative editing in Jupyter. |
for more information, see https://pre-commit.ci
I'll release v2.0.0. |
pycrdt is a new library providing Python bindings for Yrs. It speaks the "Y" protocol just like y-py, but has a different API.