-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Refactor extension to support arbitrary shell command runnables #16839
Conversation
} | ||
|
||
class CargoTaskProvider implements vscode.TaskProvider { | ||
class RustTaskProvider implements vscode.TaskProvider { |
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.
nit: I'm not sure there will be any other kind of Task run by rust-analyzer, so I wouldn't qualify with Rust
, but I also don't feel too strongly.
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 think it needs some name, class TaskProvider implements vscode.TaskProvider
would be confusing. I don't feel strongly about the name though. Do you have any suggestions? RustAnalyzerTaskProvider
is maybe a little long.
@@ -39,14 +37,15 @@ class CargoTaskProvider implements vscode.TaskProvider { | |||
{ command: "run", group: undefined }, | |||
]; | |||
|
|||
const cargoPath = await toolchain.cargoPath(); |
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 don't know if this should happen in this PR, but I wonder whether toolchain.ts
should even exist or if the toolchain contents shouldn't be synced over from rust-analyzer itself over a request, given the work that Lukas did to make toolchain discovery more reliable. there's two separate implementations of this discovery mechanism, and the one in the extension is substantially less robust.
if we're to switch over, i think it's worth considering keeping the old and new versions of runnables around before removing the older version.
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 PR (especially with #16840 being a larger change) is probably better as a small focused PR, but I agree it would be nice to follow up on how toolchains are handled :)
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.
Yes, we should ideally replicate the toolchain discovery here / allow clients to query the server for the correct toolchain stuff. Not doing that can cause recompilations and random failures. (Agree that that can go into a separate PR though)
Thanks! |
☀️ Test successful - checks-actions |
Fix tasks in tasks.json #16839 refactored the representation of tasks inside the VS Code extension. However, this data type is exposed to users, who can define their own tasks in the same format in `tasks.json` or `.code-workspace`. Revert the data type to have a `command` field rather than a `program` field, and document the different fields. This code is also a little complex, so split out a `cargoToExecution` to handle the Task to Execution conversion logic. After this change, any tasks.json with a `command` field works again. For example, the following tasks.json works as expected: ``` { "version": "2.0.0", "tasks": [ { "type": "cargo", "command": "build", "problemMatcher": [ "$rustc" ], "group": "build", "label": "my example cargo build task" } ] } ``` Fixes #16943 #16949
Currently, the extension assumes that all runnables invoke cargo. Arguments are sometimes full CLI arguments, and sometimes arguments passed to a cargo subcommand.
Refactor the extension so that tasks are just a
program
and a list of stringsargs
, and renameCargoTask
toRustTask
to make it generic.(This was factored out of #16135 and tidied.)