Parse document for tokens on every LSP on_change event #1195
Conversation
JoshuaBatty
force-pushed
the
josh/lsp_parse_document
branch
from
077950e to
55ff6b9
Compare
| session.update_text_document(¶ms.text_document.uri, params.content_changes) | ||
| ) -> Vec<Diagnostic> { | ||
| let path = params.text_document.uri.path(); | ||
| let _ = session.update_text_document(¶ms.text_document.uri, params.content_changes); |
There was a problem hiding this comment.
Looks like a Result might be getting ignored here now?
Perhaps the return type of this method should be:
-> Result<Vec<Diagnostic>, DocumentError>And this line should be:
session.update_text_document(¶ms.text_document.uri, params.content_changes)?;Otherwise if there's a reason it's ignored, we should add a comment to justify it.
There was a problem hiding this comment.
That's a fair point. The original possible error comes from using the path as the key into a HashMap of files, if the key doesn't exist an error is returned. The error is being ignored because we never want our server to crash. There is probably no point in returning an error at all then from the update_text_document fn. I'll update that fn now.
There was a problem hiding this comment.
Nice one chap, I think this approach makes a lot of sense at least until we notice re-parsing the whole document causing performance issues downstream!
When/if we do re-visit manually updating small sections of a doc for optimisation purposes, we might want to do so alongside a discussion around incremental parsing in sway-core itself.
| if let Some(ref mut document) = self.documents.get_mut(url.path()) { | ||
| changes.iter().for_each(|change| { | ||
| document.apply_change(change); | ||
| }); |
There was a problem hiding this comment.
I guess once we get some logging we could do something like log::warn!("no document for url: {}", url); or something along those lines.
There was a problem hiding this comment.
Yeah exactly, logging warnings to the console makes more sense to report errors than crashing for an LSP server.
Yeah exactly, makes a lot of sense for the functionality to come from |
see #1177 for context.
closes #1177 and closes #1147