-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Initialize and update the ydoc path property #342
Conversation
I feel like that would be a better approach, as any reloading from disk is a risk of race condition in RTC context, right? |
Thanks @brichet. |
Yes, it doesn't fix the issue on renaming the file out-of-band. It works when renaming the file using jupyterlab. |
Any opinion on how to do it ? The 2 options I can see are:
|
I think you can now remove the note in the top comment? |
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 @brichet.
Is it expected that the new test fails in the context of |
You should be able to reproduce with |
It works on my side with python 3.8 |
I think it's not only Python, but all dependencies. You can probably use |
Not sure about that, I checked the packages and they are similar to mine. It should be the |
Huh: jupyter-collaboration/projects/jupyter-collaboration/pyproject.toml Lines 32 to 35 in b8fafc0
I thought I fixed it with #335 but clearly it did not work as intended and instead of replacing the old ones just added a new line on top: which means the last one gets used |
7fb6f81
to
dfad4da
Compare
Maybe exclude [tool.mypy]
exclude=[
"^binder/jupyter_config\\.py$",
"^scripts/bump_version\\.py$",
"/setup\\.py$",
] |
Thanks @davidbrochart. Maybe we could exclude it, but do you know why it fails in this PR ? It seems to pass on recent ones... |
I don't know, I've seen that in other repos too. |
And it seems there are some other issue after the release: I will first remove the test to be able to merge this one. |
ecb7710
to
3465d1f
Compare
ddf100c
to
030db00
Compare
It looks like |
Yes, it is not related |
* Backport PR #342 to 2.x branch * linting * update snapshots * Update jupyter_collaboration/loaders.py Co-authored-by: David Brochart <david.brochart@gmail.com> --------- Co-authored-by: David Brochart <david.brochart@gmail.com>
This PR fixes #341
NOTE:
In this implementation, the document is reloaded when the path change, as if the file content has changed.Another approach could be to add a listener on the path only in theFileLoader
, which only update the path property of the ydoc on change.