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

fix: Calculate byte offset after applying each document change #304

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

radeksimko
Copy link
Member

Fixes #283

The diff is considerably large, but most of it is actually just code moving between packages and pointers being added to various places.

@radeksimko radeksimko added the bug Something isn't working label Nov 17, 2020
@radeksimko radeksimko requested a review from a team November 17, 2020 19:53
@radeksimko radeksimko added this to the v0.10.0 milestone Nov 17, 2020
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

I am a little lost on this one, but everything looks good!

@radeksimko
Copy link
Member Author

radeksimko commented Nov 18, 2020

The root cause is partially explained in the conversation in the linked issue, but here is a summary:

When receiving didChange with a slice of >1 changes, we [previously] calculated the byte offset for all of them just after receiving the request, but that can only be done for the first one, because as per LSP spec, the positions are "relative" to the state of the document after previous changes (in the batch) were applied.

Hence I moved the byte offset calculation from lsp package (which shouldn't really do this kind of stuff anyway; it just felt appropriate because in order to convert "LSP position" into HCL, you need access to the full document to calculate the byte offset) into filesystem.

@radeksimko radeksimko merged commit d6cd1cb into master Nov 18, 2020
@radeksimko radeksimko deleted the b-byte-offset-calc branch November 18, 2020 10:51
@ghost
Copy link

ghost commented Dec 18, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document synchronization between client and server goes awry
2 participants