Skip to content

Fix so code can be run after creating terminal #567

Merged
Ikuyadeu merged 2 commits intoREditorSupport:masterfrom
andycraig:run-after-create-terminal
Mar 7, 2021
Merged

Fix so code can be run after creating terminal #567
Ikuyadeu merged 2 commits intoREditorSupport:masterfrom
andycraig:run-after-create-terminal

Conversation

@andycraig
Copy link
Copy Markdown
Collaborator

Fixes #563

Fixes #483

What problem did you solve?

Trying to create a terminal and run code at the same time results in a new terminal but does not run the code. The user has to run the command again to send the code to the terminal. This PR fixes that.

The problem was that even when terminal was successfully created, success was undefined and so chooseTerminal would always return undefined. Since failure to create a terminal should result in rTerm being undefined anyway, it should be safe to remove success and always return rTerm.

How can I check this pull request?

  1. Create a file temp.R containing the code x <- 1
  2. Disable setting R: Always Use Active Terminal
  3. Run command R: Run Selection/Line. Observe that a new R terminal is created and that x <- 1 is sent to that terminal
  4. Close the R terminal by clicking the trash icon
  5. Run command R: Run Source. Observe that a new a new R terminal is created and that source(...) is sent to that terminal
  6. Close the R terminal by clicking the trash icon
  7. Check that it still fails gracefully if there is a problem creating the terminal:
    1. Change setting R: Rterm Linux/Mac/Windows to a nonsense path like 'abc'
    2. Run command R: Run Selection/Line. Observe appropriate error message 'Cannot find R client ...'

Note

This is the output I get when using R: Run Selection/Line. Note the doubled x <- 1. I could avoid that by increasing the delay amount but the amount required is presumably going to vary according to how fast the user's PC is. I've left it as-is for now. We could add a setting if it bothers people.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

x <- 1
> x <- 1
> 

Even when terminal was successfully created, success was undefined and
so chooseTerminal would return undefined. Since failure to create a
terminal should result in rTerm being undefined, it should be safe to
ignore success and always return rTerm.
@andycraig andycraig changed the title Run after create terminal Fix so code can be run after creating terminal Mar 5, 2021
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

Works as expected.

@andycraig
Copy link
Copy Markdown
Collaborator Author

@renkun-ken Thanks for the review! I was about to merge this but looking at the recent commit history there seem to be fewer merge commits than there used to be. Are we favouring 'Squash and merge' over 'Create a merge commit'?

@renkun-ken
Copy link
Copy Markdown
Member

All recent PRs were merged with "Squash and merge". @Ikuyadeu Do we have a preference on this? I'm totally ok with both if the PR is clean and short.

@Ikuyadeu
Copy link
Copy Markdown
Member

Ikuyadeu commented Mar 7, 2021

@andycraig Thank you for your great work.

@renkun-ken Thank you for your every review and discussion.

I'm totally ok with both if the PR is clean and short.

I agree with you. It depends on the reviewers' feelings.
In my opinion, two methods can be used for the following pourpose

  • Squash and Merge: Reduce the tangled commits from history to avoid hiding other commits
  • Merge: Show the separated commits for the maintenance and revert one commit

Additionally, like this PR, @andycraig 's two commits have separated works.
I think Merge is better in this case for checking code history.

@Ikuyadeu Ikuyadeu merged commit 7d4b2ac into REditorSupport:master Mar 7, 2021
@andycraig
Copy link
Copy Markdown
Collaborator Author

@Ikuyadeu @renkun-ken Thanks both!

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.

There maybe a bug with r.runSource / Run Source Have to press ctrl+enter two times to "Run Command in Terminal" when set to radian

3 participants