-
Notifications
You must be signed in to change notification settings - Fork 131
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
internal/service: add support for didChangeWatchedFiles #790
internal/service: add support for didChangeWatchedFiles #790
Conversation
Hi @danishprakash There is a few PRs I'd like to merge first before looking at this one. Specifically #839 and #843 Thank you for the patience. |
With regards to the implementation and to answer some of your questions above: I'd have to test this for real but my assumption is that So with that in mind I think that the implementation could be simplified to basically this terraform-ls/internal/langserver/handlers/did_change.go Lines 55 to 60 in 8529ae5
but of course we'd first have to filter/deduplicate it down to just directories (module paths) and decide what do we do with files which the server doesn't treat as editable (i.e. anything other than |
@danishprakash The mentioned PRs were both now merged. When you have a moment - do you mind rebasing your PR? Let me know if you need any further help beyond the answers I provided. |
dd3cae6
to
ad97566
Compare
@radeksimko thanks for the explanation. That helped a lot, however, I still have a few doubts/questions about some of those points, please correct me If I'm wrong or point me to the right resource If I seem to have missed something but basically
I understand we have
Again, I might be missing something here but based on the spec, can we say the above is an assumption at best? Finally, I'm also assuming based on the discussion above that we're going to ignore |
You are right in that there are unfortunately parts of LSP which are not explicitly documented within the spec but servers pretty much rely on certain client implementations, most often the VS Code client library. The assumptions I made above are based on the VS Code client library's behaviour. In general the spec doesn't seem to provide a lot of details on what the server should do with received documents from All I can find is this
and "read the document’s content using the document’s Uri" I'd interpret as "read the document's content from the disk/filesystem". The spec also doesn't explicitly call out how remote environments should be handled on either client or server side. I'm guessing the assumption is mostly that LSP generally won't be used in a remote environment but I haven't researched this too much. If you have some other assumptions/interpretations mismatching mine I'd be happy to discuss them! 😄 Relatedly I'm sure the LSP folks would welcome PRs with clarifications at https://github.com/microsoft/language-server-protocol
That would be my assumption as well. The only edge case I would check however is how renames/moves are handled by the client - for example:
btw. when the spec nor the VS Code client implementation provides us sufficient clarity, we consult the They also seem to implement a delay (default 100ms) and process notifications in batches, but I wouldn't implement it here yet unless we have more context and real evidence it affects our users too. |
ad97566
to
b450b15
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.
This is getting quite close. I left you some comments in-line.
Also feel free to checkout this PR branch if you need to test it: hashicorp/vscode-terraform#1095
b450b15
to
8d2f52f
Compare
8d2f52f
to
3a16266
Compare
Signed-off-by: danishprakash <grafitykoncept@gmail.com>
3a16266
to
acb603c
Compare
@danishprakash I will take this over to bring it over the finish line (while naturally retaining your authorship for existing commits), I hope you don't mind. |
@radeksimko sure, but if it's okay with you, I'd love to know what all is pending and required If I were to complete this PR? I know tests are one. Apart from that, I'd really like to understand the complications if there are any. Thanks :) |
@danishprakash Aside from the tests, which I added in the 2nd commit, the first one addresses the following:
|
Also after testing it more I just realized I was wrong about the need of checking just |
This turns out to be more complex than I initially anticipated, but I believe my last commit finally accounts for the remaining cases (deletions and creations). @danishprakash let me know if you have any feedback or questions. |
@radeksimko hey, I have a question about the recent updates:
|
fi, err := os.Stat(parentDir) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
// if not, we remove the indexed module |
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.
When can this happen? The user deleted the only file and then subsequently the directory, too? If yes, then those would also be different events here and that's probably why you're just ensuring the directory is still present or not. Is my understanding correct?
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.
This accounts for the deletion of a whole directory.
If yes, then those would also be different events here
Frankly, I don't know, since LSP doesn't go into detail about how clients should or shouldn't group file events. In fact when I tried to rm -rf submodule
in VS Code where submodule had some *.tf
files, no events were fired at all - which may be a bug in the VS Code language client - unsure.
So in the face of uncertainty I'm just being a little more defensive here.
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.
Got it, thanks for the explanation and yes, it's still a little nebulous when it comes to understanding client-side event emits.
may be a bug in the VS Code language client
Do you mean https://github.com/Microsoft/vscode-languageserver-node?
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.
It could be either VS Code itself or the language client you linked and it may also be OS-specific problem since file watching is highly dependent on the OS and implementations/behaviours can differ between systems - it would take some debugging to find this out.
svc.logger.Printf("error checking existence (%q deleted): %s", parentDir, err) | ||
continue | ||
} | ||
if !fi.IsDir() { |
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.
Curious as to why this check is here since we're doing this earlier:
parentDir := filepath.Dir(rawPath)
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.
Right, this should basically never happen - I was just trying to be a bit more defensive here too.
The only realistic scenarios which come to mind involve cases where it's not a file nor a directory, but e.g. symlink or any special type of file - which is mostly relevant on Unix-based systems:
https://pkg.go.dev/io/fs#FileMode
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.
Okay, so just yesterday, I got bit by a severe production bug wherein I was copying files from one directory to another. Without a safety check of whether the file is a regular file or not, we ended up doing a cp on a file which was a UNIX pipe and the application just froze indefinitely waiting for the copy to finish which would never be the case. I can totally understand the need for this check here howsoever obvious it might seem.
So |
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.
I'm still nervous about opening so many message windows that they can't silence, for things the user can't do anything about, but this all works for the cases I could think of so I don't have any objections to merging this. Thank you both for this work!
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Closes #15