fix bug in 'switchImplIntf': #254
Conversation
| let possible_filepath = Uri.to_path file_uri in | ||
| if possible_filepath = Uri.to_string file_uri then | ||
| (* remove URI scheme *) | ||
| String.split ~on:':' possible_filepath |> List.tl |> List.hd |
There was a problem hiding this comment.
How about just:
match String.split with
| _ :: x :: _ - >x
| _ -> assert false
Does that mean there's no lsp assistance until the file is saved? |
Yes; extension understands that's an OCaml file but we miss ocamllsp support for that file. We could artificially send didOpen notif when that unsaved file is created, but we'd have to handle cases: what happens when it's saved, what happens when it's closed without saving, can merlin handle files with non file URIs, etc. |
Nah, there's no need. I was just curious if this was indeed the case. It seems like a flaw in LSP itself and we shouldn't bother patching over it. |
one couldn't switch from a new file that's not saved on disk (in vscode it's a file with URI scheme 'untitled') The previous switching logic depended on document store, which only knows files that were opened with a 'textDocument/didOpen' notification. VS Code doesn't send such notifications for unsaved files, hence the bug. Now switching handled file URIs directly without dependence on document store, which works for any 'switchImplIntf' request with a valid URI.
* add a test to switch from file uri with non-file scheme
ulugbekna
force-pushed
the
fix-switch-bug
branch
from
1834067
to
4bd1301
Compare
CHANGES: ## Features - Implement a command to switch between module interfaces and implementations (ocaml/ocaml-lsp#254) ## Fixes - Do not crash on invalid positions (ocaml/ocaml-lsp#248) - add missing record fields to list of completions (ocaml/ocaml-lsp#253) - do not offer `destruct` as a code action in interface files (ocaml/ocaml-lsp#255)
Fixes ocamllabs/vscode-ocaml-platform#382:
one couldn't switch from a new file that's not saved on disk
(in vscode it's a file with URI scheme 'untitled')
The previous switching logic depended on document store, which only
knows files that were opened with a 'textDocument/didOpen'
notification. VS Code doesn't send such notifications for unsaved files,
hence the bug. Now switching handled file URIs directly without
dependence on document store, which works for any 'switchImplIntf'
request with a valid URI.
Note: handling of URIs is brittle as is the current URI module. I would strongly be in favor of adding a vendored dep
ocaml-uri.TODO:
resultscheme:/path)