Skip to content

adding devtools tasks to command palette#880

Merged
ElianHugh merged 4 commits intoREditorSupport:masterfrom
alex-gable:add-check-to-commands
Dec 14, 2021
Merged

adding devtools tasks to command palette#880
ElianHugh merged 4 commits intoREditorSupport:masterfrom
alex-gable:add-check-to-commands

Conversation

@alex-gable
Copy link
Copy Markdown
Contributor

What problem did you solve?

while the majority of devtools::* package develop/build shortcuts were available, devtools::check() was missing.

(If you have)Screenshot

R: Check available in Command Palette and sends to R console

(If you do not have screenshot) How can I check this pull request?

Build version > Cmd + Shift + P > Type R: Check > Press Enter

@ElianHugh
Copy link
Copy Markdown
Collaborator

I wonder if it would be worth moving away from the runTextInTerm paradigm for these commands (I.e. commands that don't influence/affect the R environment). Is there a benefit to running in the terminal as opposed to a task? We have the devtools::check task definition but iirc no command palette bbinding

@renkun-ken
Copy link
Copy Markdown
Member

I think it should be a task instead of sending code to terminal.

@alex-gable
Copy link
Copy Markdown
Contributor Author

alex-gable commented Nov 27, 2021

I believe that's how RStudio treats it as well, for whatever that's worth. I totally agree that pattern makes more sense. Should that be in the scope of this PR though?

@ElianHugh
Copy link
Copy Markdown
Collaborator

I think for this PR, it would probably be worth pointing the command palette to the task binding. We can do the same for the other command palette bindings in a future PR

@alex-gable
Copy link
Copy Markdown
Contributor Author

alex-gable commented Nov 27, 2021

Understood. Just to be transparent, that's likely beyond my skill level

@ElianHugh
Copy link
Copy Markdown
Collaborator

No worries, you're 90% there already! I think you would only need to change the command and its arguments. E.g., in my keybindings I have the following:

{		
  "key": "ctrl+shift+t",		
  "command": "workbench.action.tasks.runTask",		
  "args": "R: Test"
},

@alex-gable alex-gable force-pushed the add-check-to-commands branch from 6c8f3a1 to 7cffeaf Compare November 29, 2021 04:05
@alex-gable
Copy link
Copy Markdown
Contributor Author

okay, I think this is the solution you alluded to, @ElianHugh, I appreciate the pointer. I did a bit of research and there's definitely more complex (and I think appropriate) ways to accomplish this. see: microsoft/vscode#40758.

any how. I took a couple of liberties in terms of naming and organization. I wanted to be sure the commands that match Check, which execute outside of the Terminal, all had tasks. so I added a task for Build and then grouped these task commands separately from the other editor independent commands.

I then categorized these "Package" command contributions separately in the because, as I was testing, it made the command in the palette clearer.

Feel free to revert/change whatever!

Comment thread package.json
@ElianHugh
Copy link
Copy Markdown
Collaborator

Thank you for the nice work you've done! The commands work nicely (barring the one issue I've noted above), and I think the naming is appropriate too. I don't have a strong opinion re: naming the commands r.commandTask, but it may cause unexpected issues for VSCode-R users that have shortcuts pointing to the old command names (e.g. r.test vs r.testTask)

@ElianHugh ElianHugh self-requested a review December 1, 2021 13:34
Copy link
Copy Markdown
Collaborator

@ElianHugh ElianHugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ElianHugh
Copy link
Copy Markdown
Collaborator

Anything else to add? @renkun-ken

@ElianHugh ElianHugh changed the title adding devtools::check() to command palette adding devtools tasks to command palette Dec 2, 2021
@ElianHugh ElianHugh merged commit a5725e8 into REditorSupport:master Dec 14, 2021
@renkun-ken
Copy link
Copy Markdown
Member

LGTM sorry for late reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants