Skip to content

Remove command Run Selection/Line in Active Terminal#409

Merged
andycraig merged 7 commits intoREditorSupport:masterfrom
andycraig:remove-active
Sep 16, 2020
Merged

Remove command Run Selection/Line in Active Terminal#409
andycraig merged 7 commits intoREditorSupport:masterfrom
andycraig:remove-active

Conversation

@andycraig
Copy link
Copy Markdown
Collaborator

@andycraig andycraig commented Sep 13, 2020

Closes #306

What problem did you solve?

This PR makes two changes:

  1. It completely removes command R: Run Selection/Line in Active Terminal. Current behaviour of this command is to show a window stating that the command is being removed.
  2. It refactors and simplifies code used to choose the terminal and send text to it. Currently, most commands that send text to the terminal explicitly choose the terminal too. This was to enable R: Run Selection/Line in Active Terminal to always choose the active terminal. Since that command is being removed, the code that chooses the terminal can be moved further down the sequence of function calls. Making this change means that runTextInTerm() and chooseTerminalAndSendText() do the same thing, although chooseTerminalAndSendText() works for only one line of code, so I removed chooseTerminalAndSendText().

How can I check this pull request?

  1. Check command Run Selection/Line in Active Terminal does NOT exist. (E.g., Ctrl+Shift+P, type 'Run Selection/Line in Active Terminal', confirm there is no such command.)

This PR involves code paths from many commands. It should not change the behaviour of any of them. Here is how to test they still work.

Add the following to keybindings.json:

[
    {
        "key": "ctrl+1",
        "command": "r.runCommand",
        "when": "editorTextFocus",
        "args": "TEST runCommand"
    },
    {
        "key": "ctrl+2",
        "command": "r.runCommandWithSelectionOrWord",
        "when": "editorTextFocus",
        "args": "TEST runCommandWithSelectionOrWord $$"
    },
    {
        "key": "ctrl+3",
        "command": "r.runCommandWithEditorPath",
        "when": "editorTextFocus",
        "args": "TEST runCommandWithEditorPath $$"
    }
]

Make a directory r-test containing a file temp.R, with this content:

1
print("hello world")

Place cursor on word 'hello'.

I recommend doing the below once with setting r.alwaysUseActiveTerminal enabled and once with it disabled.

  1. Run command R: Run Source. Confirm source([file path]) is sent to terminal.
  2. Run command R: Knit Rmd. Confirm file is knitted in terminal.
  3. Run command R: Run Selection/Line. Confirm print("hello world") is sent to terminal.
  4. Run command R: Run Selection/Line (Retain Cursor). Confirm print("hello world") is sent to terminal.
  5. Run command R: Show length for a selected object. Confirm length(hello) is sent to terminal. (It's not a variable so it will cause an error - that's fine.)
  6. Press Ctrl+1. Confirm TEST runCommand is sent to terminal. (It's not valid R code so it will cause an error - that's fine.)
  7. Press Ctrl+2. Confirm TEST runCommandWithSelectionOrWord hello is sent to terminal. (It's not valid R code so it will cause an error - that's fine.)
  8. Press Ctrl+3. Confirm TEST runCommandWithEditorPath [file path] is sent to terminal. (It's not valid R code so it will cause an error - that's fine.)
  9. Place cursor on line 1, run command R: Run from Beginning to Line. Confirm 1 is sent to terminal.
  10. Run command R: Load All. Confirm devtools::load_all() is sent to terminal. (It will cause an error - that's fine.)
  11. Run command R: Test Package. Confirm devtools::test() is sent to terminal. (It will cause an error - that's fine.)
  12. Run command R: Install Package. Confirm devtools::install() is sent to terminal. (It will cause an error - that's fine.)
  13. Run command R: Build Package. Confirm devtools::build() is sent to terminal. (It will cause an error - that's fine.)
  14. Run command R: Document. Confirm devtools::document() is sent to terminal. (It will cause an error - that's fine.)
  15. Run command R: Attach Active Terminal. Confirm .vsc.attach() is sent to terminal.
  16. Run command R: Preview Environment. Confirm something starting with write.csv is sent to the terminal. (This command is buggy so it might produce an error.)
  17. Run command R: Preview Dataframe. Confirm something starting with write.csv is sent to the terminal. (hello is not a variable, so it might produce an error because of that, or it might produce a different error because it the command is buggy.)

@andycraig
Copy link
Copy Markdown
Collaborator Author

Thank you CI! I'd missed a couple of uses of chooseTerminalAndSendText(). I've fixed those, and all CI checks are passing now. I've added three more suggested tests to the main PR text.

@renkun-ken
Copy link
Copy Markdown
Member

Thanks for the work! I'll do some testing soon.

@renkun-ken
Copy link
Copy Markdown
Member

I've tested all above items and everything works nicely.

Please feel free to merge if nothing is to be added.

Copy link
Copy Markdown
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

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

LGTM

@andycraig
Copy link
Copy Markdown
Collaborator Author

@renkun-ken Thank you for testing and reviewing!

@andycraig andycraig merged commit 2fbf97b into REditorSupport:master Sep 16, 2020
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.

Remove Run Selection/Line in Active Terminal

2 participants