-
Notifications
You must be signed in to change notification settings - Fork 91
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
R package build / check / etc as tasks provided by extension #735
Conversation
I think for an internal alpha it is OK to assume folks are devtools users. In the longer run, we will want to provide some kind of implementation for non-devtools users. |
A follow-up to this PR will be to bind these tasks to commands, and then key bindings. |
For commands that are meant to be run in the current R session (e.g. |
In the longer run, we could use the problem matchers to surface problems from, say, checking, but for now I've turned them off. |
We chatted and decided that tasks are the right decision for these kinds of needs. I believe this has everything in it now for an initial change. After this PR is merged, in the short term, I will:
In the long term, we can:
|
export async function providePackageTasks(_context: vscode.ExtensionContext): Promise<void> { | ||
|
||
const isRPackage = await detectRPackage(); | ||
vscode.commands.executeCommand('setContext', 'isRPackage', isRPackage); |
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 put this here instead of commands.ts
because it is executing the "setContext" command, not registering a command for users to have.
vscode.commands.executeCommand('setContext', 'isRPackage', isRPackage); | ||
|
||
const allPackageTasks: PackageTask[] = [ | ||
{ 'type': 'rPackageLoad', 'name': 'Load package', 'shellExecution': 'R -e "devtools::load_all()"' }, |
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.
The new executeCode()
API is now available if you want to use it for tasks that should execute in the current R session. (I think that's at least load_all
?)
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 was going to do these two things (includes switching execution, yes) in immediate followup PRs after this one, if that is OK:
After this PR is merged, in the short term, I will:
- make commands and bind them to these tasks (including key bindings)
- change the execution to use Add simple API for executing code in Positron #747
The custom execution for tasks is just a bit more complicated and I wanted to approach it separately, unless you feel 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.
Absolutely fine to handle this in a followup!
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!
Addresses #28 and #122
The biggest question is whether implementing this as a task is the right call. I believe the main alternative is a command.
This is currently an initial POC and still needs:
devtools::load_all()
, checking, and probably simple testing as wellimplement optional(only need when there is no execution but all our tasks haveresolveTask()
for a better command palette experienceexecution
)