feat: URI Handler #431
feat: URI Handler #431
Conversation
catskul
requested review from
coeuvre,
jfirebaugh,
cameron-martin and
vogelsgesang
as code owners
catskul
force-pushed
the
catskul/uri-handler
branch
4 times, most recently
from
2fa1f1c to
4d78ea4
Compare
| vscode.window.registerUriHandler({ | ||
| async handleUri(uri: vscode.Uri) { | ||
| try { | ||
| const workspace = vscode.workspace.workspaceFolders[0]; |
There was a problem hiding this comment.
presume it's in the first workspaceFolder. I suppose something more sophisticated could be done, but not sure it's necessary. Happy to accept suggested change here if someone has a specific suggestion.
There was a problem hiding this comment.
Without any way of identifying the bazel workspace, I don't think we can do any better than assume the first workspace, so I'm happy with this.
catskul
force-pushed
the
catskul/uri-handler
branch
from
4d78ea4 to
b222b07
Compare
|
formatting fixes pushed. |
|
What's the use case for this? |
|
@cameron-martin When targets fail, esp in cloud CI, notifications are often sent via slack, email, etc. The use case here is to make it possible to provide clickable links right into vscode. This is directly analogous to VSCode's more general ability to link to files in vscode: |
| @@ -19,15 +19,48 @@ import { | |||
| Position, | |||
| TextDocument, | |||
| Uri, | |||
| window, | |||
There was a problem hiding this comment.
Both these extra imports seem to be unused.
| getDefaultBazelExecutablePath(), | ||
| Utils.dirname(document.uri).fsPath, | ||
| ).queryTargets(`kind(rule, "${targetName}") + kind(file, "${targetName}")`); | ||
| const location = await targetToUri(targetText, Utils.dirname(document.uri)); |
There was a problem hiding this comment.
I'm not sure if this case can ever realistically get hit, but to make this refactor preserve behaviour you should return null here if location is null.
There was a problem hiding this comment.
Fixed with ternary. Let me know if you'd rather I used some other format.
| vscode.window.registerUriHandler({ | ||
| async handleUri(uri: vscode.Uri) { | ||
| try { | ||
| const workspace = vscode.workspace.workspaceFolders[0]; |
There was a problem hiding this comment.
Without any way of identifying the bazel workspace, I don't think we can do any better than assume the first workspace, so I'm happy with this.
|
Will try to update this tonight. |
catskul
force-pushed
the
catskul/uri-handler
branch
from
22974ba to
431e319
Compare
|
All comments addressed. Happy to make any additional changes if you see any other issues. |
Putting my code where my mouth is:
#430
This feature will allow us to add links to documents, and tool output which jumps to specific targets, reducing friction and wasted time.
URI handler works such that:
vscode://bazelbuild.vscode-bazel//my/path/to/tests:target
(unfortunately github itself won't link out to non-http urls)
Will jump to said target given vscode is opened to a workspace which contains a bazel workspace with that target as the first
workspaceFolderI imagine we could handle more situations like:
vscode://bazelbuild.vscode-bazel/fspath/to/workspace//my/path/to/tests:target
or
vscode://bazelbuild.vscode-bazel/test//my/path/to/tests:target
But I imagine anything of the sort would be more controversial.