internal: Clean up runnable lsp extension#17547
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
changed the title
| /** | ||
| * Environment variables to set before running the command. | ||
| */ | ||
| environment?: Record<string, string>; |
There was a problem hiding this comment.
Open to alternative names, should we be explicit with environmentVariables?
There was a problem hiding this comment.
could also call it envVariables?
There was a problem hiding this comment.
Eh, I'll stick with environment I think.
There was a problem hiding this comment.
fair enough! not terribly attached.
| /** | ||
| * The working directory to run the command in. | ||
| */ | ||
| cwd: string; |
There was a problem hiding this comment.
I feel like this being mandatory gives a more unique experience across clients. We should not leave this decision to clients as that could result in runnables sometimes working sometimes not. (notable it is required by the shell version already)
There was a problem hiding this comment.
yeah, reducing the number of possible options is great. the combinatorial explosion of options made this really difficult to reason about.
|
r? @davidbarsky |
davidbarsky
left a comment
davidbarsky
left a comment
There was a problem hiding this comment.
this looks good and reasonable to me.
|
@bors r+ |
internal: Clean up runnable lsp extension This feels like a natural addition to me, and also allows us to drop the expect-test hardcoding from the extension. Additionally, `cargoExtraArgs` is pointless, all the client will do is merge it with `cargoArgs` so the server can do that instead of delegating that to the client.
|
@bors r+ |
|
I'm a bit confused by #4604 (comment), in particular:
Does this refer to |
It does not. It refers to the field in the runnable sent from the server to the client. |
|
It refers to this rust-analyzer/docs/dev/lsp-extensions.md Line 382 in 78d5f05 rust-analyzer.cargo.extraArgs config yes. rust-analyzer.cargo.args isn't a thing, as the cargoArgs in the runnable is populated by the server depending on the task, its not something the user configures.
|
|
☀️ Test successful - checks-actions |
1 similar comment
|
☀️ Test successful - checks-actions |
|
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
|
Thanks a lot for the clarification! |
See rust-lang/rust-analyzer#17547 * eglot-x.el (eglot-x--run-after-jump): Handle new property "environment". Still accept old properties to keep supporting old versions of rust-analyzer.
5 participants
This feels like a natural addition to me, and also allows us to drop the expect-test hardcoding from the extension. Additionally,
cargoExtraArgsis pointless, all the client will do is merge it withcargoArgsso the server can do that instead of delegating that to the client.