-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change displayBuildTarget to listBuildTargets #893
Conversation
0f07b9f
to
dd75362
Compare
Needs scalameta/metals-languageclient#419 to pass checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one small suggestion in regards to parsing, but it can be ignored.
Otherwise, let's bump the language client and default metals server version once the PR in metals is merged (which I will do in a sec.)
src/extension.ts
Outdated
if (args.length > 0) { | ||
// get build target name from treeview uri of the form "projects:file:/root/metals/.mtags/?id=mtags3!/_root_/," | ||
const treeViewUri = args[0] as string; | ||
const buildTargetRegEx = new RegExp("projects:.*?id=(.*)!.*", "g"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead parse it as an URI and get the query parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still only get us halfway there.
A URI example from the treeview would be...
projects:file:/root/metals/.mtags/?id=mtags3!/_root_/
Using URLSearchParams(uri).get("id")
would give...
mtags3!/_root_/
So I'd still need to parse for !
to get mtags3
I can do that if you want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could then just do split("!")
so maybe it's a bit clrearer than a regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This splitting part seems too much for me.
IMO in this case regex is simpler and with the proper example in the comment (exists) isn't also that hard to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm easy. Ideally the treeviewuri would be in the format we already want but it's used to work out how to drill down into classes/methods so I wasn't going to change it. I tried to extract the target name with URLSearchParams
but it doesn't like our URI so converts the whole thing to a single query param which is no help. I guess URI != URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok then, for me whichever is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed to split
+ split
. Does the job. 🤷
556c70d
to
fa94a85
Compare
src/extension.ts
Outdated
client | ||
.sendRequest(ExecuteCommandRequest.type, { | ||
command: ServerCommands.ListBuildTargets, | ||
}) | ||
.then(async (...args: any[]) => { | ||
const targets = args?.[0] as string[]; | ||
window.showQuickPick(targets).then((target) => { | ||
if (target) { | ||
displayBuildTarget(target); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client | |
.sendRequest(ExecuteCommandRequest.type, { | |
command: ServerCommands.ListBuildTargets, | |
}) | |
.then(async (...args: any[]) => { | |
const targets = args?.[0] as string[]; | |
window.showQuickPick(targets).then((target) => { | |
if (target) { | |
displayBuildTarget(target); | |
} | |
}); | |
}); | |
const targets = await client.sendRequest(ExecuteCommandRequest.type, { | |
command: ServerCommands.ListBuildTargets, | |
}); | |
const picked = await window.showQuickPick(targets); | |
if (picked) { | |
displayBuildTarget(picked); | |
} |
Should work the same and, at least for me, it's more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the change. Works for me. I don't know typescript well enough to know the implications.
fa94a85
to
d53c4d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
see scalameta/metals#3649
This shifts the burden of quickpicking from Metals onto the client