-
Notifications
You must be signed in to change notification settings - Fork 22
Perform granular updates if reloading textual files #366
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
Perform granular updates if reloading textual files #366
Conversation
jupyter_ydoc/yunicode.py
Outdated
|
|
||
| # for very different strings, just replace the whole content; | ||
| # this avoids generating a huge number of operations | ||
| if matcher.ratio() < 0.6: |
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.
Using 0.6 because:
As a rule of thumb, a ratio() value over 0.6 means the sequences are close matches
As per https://docs.python.org/3/library/difflib.html#sequencematcher-examples
Instead of ratio() we could use quick_ratio() or real_quick_ratio(). In case if the ratio is above the threshold this does not matter because we would end up computing matching_blocks anyways (this is computed and cached by a call to either raito() or get_opcodes(); in that case calling quick_ratio heuristic could actually end up taking more time as we end up doing more work. It all boils down to whether we think that we are more likely to see smaller updates or larger updates. One very cheap heuristic could be using len of both strings which is what real_quick_ratio does.
davidbrochart
left a 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.
That's great, I just have minor comments.
jupyter_ydoc/yunicode.py
Outdated
| operations = matcher.get_opcodes() | ||
| offset = 0 | ||
| for tag, i1, i2, j1, j2 in operations: | ||
| if tag == "replace": |
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.
Since we require python 3.10+, maybe use match?
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.
Done in b61975ef55b4e8011016e7fbdd83f601e5ae84df.
jupyter_ydoc/yunicode.py
Outdated
| del self._ysource[i1 + offset : i2 + offset] | ||
| offset -= i2 - i1 | ||
| elif tag == "insert": | ||
| self._ysource[i1 + offset : i2 + offset] = value[j1:j2] |
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.
Why is this not using Text.insert?
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.
Changed to Text.insert in ca32120.
as `__setitem__` also checks if index is a number or slice and then checks the range of the slice; we can skip those knowing that `i1 == i2` in the `insert` opcode.
Uh oh!
There was an error while loading. Please reload this page.