New runnables #4710
New runnables #4710
Conversation
|
✌️ vsrs can now approve this pull request. To approve and merge a pull request, simply reply with |
| } | ||
| ``` | ||
|
|
||
| rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look like this: |
There was a problem hiding this comment.
Maybe it makes sense to distinguish different cargo commands?
I believe test targets, binaries and utility commands like 'check' should be different things. Anyway on the client side we have to filter them here and there.
export interface BinaryArgs { /*...*/ }
export interface TestArgs { /*...*/ }
export interface CheckArgs { /*...*/ }
export interface ExpandArgs { /*...*/ }
export interface Runnable {
label: string;
location?: lc.LocationLink;
kind: "cargo";
workspaceRoot?: string;
args: BinaryArgs | TestArgs | CheckArgs | ExpandArgs;
}There was a problem hiding this comment.
I’ve thought about this, but decided that looking at the cargoArgs[0] is probably good enough
There was a problem hiding this comment.
To me the main benefit of having different XXXArg kinds is that it would be easier and less error prone to get, for example, test id or expandable function name, etc. No need to parse raw string array and write client side tests :)
There was a problem hiding this comment.
Better use a discriminated union with kind discriminator. Types are our friends, not enemies...
any is generally bad practice and lsp has it just because it is legacy (e.g. unknown is called to replace it), bear with me...
There was a problem hiding this comment.
It can't be a discritimated union, because it needs to be open. We'll use unknown once the LSP spec uses unknown.
I've though about it, and I see how for Cargo case we might want to use BinaryArgs | TestArgs in principle, but:
- it seems like we don't need those right now
- it seems like when we add support for non-cargo based build systems, the specific layout here will change.
So, I think I'd stick with the simplest possible thing for now.
| ```typescript | ||
| interface RunnablesParams { | ||
| textDocument: TextDocumentIdentifier; | ||
| /// If null, compute runnables for the whole file. |
There was a problem hiding this comment.
| /// If null, compute runnables for the whole file. | |
| /// If not present, compute runnables for the whole file. |
There was a problem hiding this comment.
If you intended for it to possibly be null, then it better be position?: null | Position;
There was a problem hiding this comment.
I believe protocol is using | null for semantcially similar cases.
| } | ||
| ``` | ||
|
|
||
| rust-analyzer supports only one `kind`, `"cargo"`. The `args` for `"cargo"` look like this: |
There was a problem hiding this comment.
Better use a discriminated union with kind discriminator. Types are our friends, not enemies...
any is generally bad practice and lsp has it just because it is legacy (e.g. unknown is called to replace it), bear with me...
|
bors r+ |
4769: Fix Run lens. r=matklad a=vsrs This PR fixes a bug introduced in #4710: second and all subsequent clicks on the Run lens produce invalid commands: 1. `cargo test --package ra_ide --lib -- hover::tests::test_hover_enum_has_impl_action --exact --nocapture` 2. `cargo test --package ra_ide --lib -- hover::tests::test_hover_enum_has_impl_action --exact --nocapture` **`-- hover::tests::test_hover_enum_has_impl_action --exact --nocapture`** 3. `cargo test --package ra_ide --lib -- hover::tests::test_hover_enum_has_impl_action --exact --nocapture` **`-- hover::tests::test_hover_enum_has_impl_action --exact --nocapture -- hover::tests::test_hover_enum_has_impl_action --exact --nocapture`** Co-authored-by: vsrs <vit@conrlab.com>
bors d=@vsrs