Skip to content

Conversation

@ElianHugh
Copy link
Collaborator

@ElianHugh ElianHugh commented Sep 10, 2021

Resolves #781

  • Moves R process commands from inline text to R scripts
  • Use environment variables to pass values to the scripts (as per @renkun-ken's suggestion)
  • No user-facing changes

Why?

In general, easier to maintain:

  • Easier to read
  • Easier to test!
  • Less pain re: quoting
  • More extensible

Changes

  • Move R-related session files to R/session
  • Move getAliases.R to R/help
  • New file helpServer.R in R/help
  • New file knit.R in R/rmarkdown
  • New file preview.R in R/rmarkdown

  • I looked at other uses of childProcesses, but I'm not 100% at what point we should use a file vs. inline text.
  • Tested on WSL (Ubuntu) and Arch Linux

- Moves R process commands from inline text to R scripts
- Use environment variables to pass values to the scripts
@renkun-ken
Copy link
Member

Should we also make it for the R help server at

const cmd = (
`${this.rPath} --silent --slave --no-save --no-restore -e ` +
`"cat('${lim}', tools::startDynamicHelp(), '${lim}', sep=''); while(TRUE) Sys.sleep(1)" `
);

@ElianHugh
Copy link
Collaborator Author

Should we also make it for the R help server at

const cmd = (
`${this.rPath} --silent --slave --no-save --no-restore -e ` +
`"cat('${lim}', tools::startDynamicHelp(), '${lim}', sep=''); while(TRUE) Sys.sleep(1)" `
);

I wasn't sure at what point/complexity we should create files. Do we think all background processes?

@renkun-ken
Copy link
Member

I think we should put a script if the command line has multiple expressions or needs quoting.

@renkun-ken
Copy link
Member

Also, do we really need the background folder? Will the following look better or more confusing?

  • R/help/getAliases.R
  • R/help/server.R
  • R/rmarkdown/knit.R
  • R/rmarkdown/preview.R

@ElianHugh
Copy link
Collaborator Author

Also, do we really need the background folder? Will the following look better or more confusing?

* `R/help/getAliases.R`

* `R/help/server.R`

* `R/rmarkdown/knit.R`

* `R/rmarkdown/preview.R`

I was thinking the background folder, because I wanted to distinguish it from the session-watcher related files. Would it make more sense to have the following?

  • R/rmarkdown/
  • R/help/
  • R/session/

@renkun-ken
Copy link
Member

Would it make more sense to have the following?

Makes good sense.

- change require to requireNamespace
- Move session related R files to R/session/
@ElianHugh
Copy link
Collaborator Author

I've moved the session files to R/session! Everything should work as before -- just double checking if I caught everything

Copy link
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.

Everything works well.
LGTM

@ElianHugh ElianHugh merged commit 055a87e into REditorSupport:master Sep 10, 2021
ElianHugh added a commit to ElianHugh/vscode-R that referenced this pull request May 12, 2022
* Move R-related session files to `R/session`
* Move `getAliases.R` to `R/help`
* New file `helpServer.R` in `R/help`
* New file `knit.R` in `R/rmarkdown`
* New file `preview.R` in `R/rmarkdown`
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.

Consider storing background-process R commands in files

2 participants