lsp: Fix missing initial root#1692
Conversation
Fixes open-policy-agent#1671 Signed-off-by: Charlie Egan <charlie_egan@apple.com>
anderseknert
left a comment
anderseknert
left a comment
There was a problem hiding this comment.
Thanks! Looks very good to me. Just some nits and a suggestion.
| if l.workspaceRootURI == "" { | ||
| err := l.updateRootURI(ctx, | ||
| // get the URI of the file's immediate parent | ||
| uri.FromPath(l.client.Identifier, filepath.Dir(uri.ToPath(l.client.Identifier, params.TextDocument.URI))), |
There was a problem hiding this comment.
You can use l.fromPath and l.toPath here
| } else { | ||
| l.log.Message("no config file found for workspace") | ||
| } | ||
| configRoots, err := lsconfig.FindConfigRoots(uri.ToPath(l.client.Identifier, normalizedRootURI)) |
There was a problem hiding this comment.
Same here and later in this function.
| if l.workspaceRootURI != "" { | ||
| workspaceRootPath := l.workspacePath() | ||
| if params.RootURI != "" { | ||
| err := l.updateRootURI(ctx, params.RootURI) |
|
|
||
| if l.workspaceRootURI != "" { | ||
| workspaceRootPath := l.workspacePath() | ||
| if params.RootURI != "" { |
There was a problem hiding this comment.
Should we also look for this in the workspaceFolders param?
The spec suggests that's where it should be, and we've mostly been lucky that clients don't seem to honor that https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initializeParams
(and it's easy to see why they don't given how poorly that is documented)
Not suggesting we handle multiple workspaces as part of this, but we could perhaps just check if there's something there, and if there is we go with the first one (if nothing else was given).
There was a problem hiding this comment.
yeah that's fair, I have something in there now
There was a problem hiding this comment.
noted in a comment that it's untested but based on the spec
|
|
||
| files := map[string]string{ | ||
| "foo/main.rego": mainRegoContents, | ||
| // here we are ignoring two issues with the file, so we expect no to |
| mainRegoURI := uri.FromPath(clients.IdentifierGeneric, filepath.Join(tempDir, filepath.FromSlash(mainRegoFileName))) | ||
|
|
||
| receivedMessages := make(chan types.FileDiagnostics, defaultBufferedChannelSize) | ||
| clientHandler := test.HandlerFor(methodTdPublishDiagnostics, test.SendsToChannel(receivedMessages)) |
There was a problem hiding this comment.
Humble brag, but these helpers are so nice!
There was a problem hiding this comment.
yeah, big improvement
Address other PR comments. Signed-off-by: Charlie Egan <charlie_egan@apple.com>
|
Thanks for the review, I have made a number of edits in a second commit: 60e9e01 |
2 participants
Fixes #1671
when there is a user level config
when there is no config at all
