adjust selectionRange API #474
Conversation
| if (selectionRange.parent) { | ||
| parent = this.asSelectionRange(selectionRange.parent); | ||
| if (!parent || !(parent.range.contains(range) && !parent.range.isEqual(range))) { | ||
| return undefined |
There was a problem hiding this comment.
What is the proper way to report invalid data? eg, when parent is smaller than a child?
There was a problem hiding this comment.
Yeah, child must be contained by parents
There was a problem hiding this comment.
Yep, and this code checks this conditions. The question is, what to do if the server implementation returns wrong data?
The behavior should be such that:
- server implementer ideally should see a loud error and fix the problem, instead of spending time with debugger to figure out where exactly the info is dropped
- editor user should ideally get some message (such that they can open an issue for the corresponding extension), but it should not be distracting, and it should not be repeating.
Is there some function like show_error_dialog_if_in_dev_mode_otherwise_log_a_warning?
| } | ||
| return new VSelectionRange(range, parent); | ||
| } | ||
| } | ||
|
|
||
| declare module 'vscode' { |
There was a problem hiding this comment.
note that the API is not proposed in insiders, so we should remove this once we update vscode dependency
|
I created a branch with the updated API Regarding error checking:
Let me know if the changes look OK? |
|
Sorry for the overlap but this PR was closed / merged for me :-) |
|
Changes in https://github.com/Microsoft/vscode-languageserver-node/tree/dbaeumer/selectionRange look good to me! I would prefer if client did more validation/error reporting, to make it easier to debug a buggy server, and to make protocol more strict, but this would be only a small quality of life improvement, nothing too important. |
|
I see the point. Could we do the following. I merge the update into master and then we take it from there? |
|
I wasn't clear enough: I would be more more than happy to have any version of this in master, I don't think validation should block anything (or, for that matter, I am only 80% sure that it is worth having validation at all). |
|
OK. Great. |
|
I guess this can be closed? The impl is already im master! |
|
Yes. Thanks for closing. |
Tested manally with insiders code and this commit in rust-analyzer. Seems to work :)