search

bnosac / cronR

A simple R package for managing your cron jobs.
Other
288 stars 38 forks source link

fix: set default RscriptRepository path to current working directory #40

Closed hedsnz closed 3 years ago

hedsnz commented 3 years ago

If no RscriptRepository path is provided to cron_rstudioaddin, the environment variable CRON_LIVE will be tried. If that's not set, it defaults to the current working directory. Removes previous process of writing to the package's extdata folder.

jwijffels commented 3 years ago

a few remarks

hedsnz commented 3 years ago

@jwijffels I've added some documentation to the README, and set the default argument of the function call. In terms of the app failing if the path doesn't exist -- a warning will be printed to the console based on the input observer, in the same way as previously. I also added an extra stop check that triggers if the path doesn't exist; that will close the app (in the same way as the check for file.copy a few lines below).

To be clear, I haven't modified any of the underlying functions, I've only changed cron_rstudioaddin. Everything appears to be working correctly through the addin, so the updated RscriptRepository appears to be passing through to the underlying functions without any issue. But there are virtually no tests and I'm not really familiar enough with the package to know where to start with that. I'm happy to have a closer look over the next couple of weeks and try to come up with something if you'd like.

jwijffels commented 3 years ago

I think you need to check whether the user has write access rights to that CRON_LIVE folder as well.

hedsnz commented 3 years ago

I think that is covered by the existing code?

https://github.com/heds1/cronR/blob/rscript-path-patch/R/cron_rstudioaddin.R#L185

jwijffels commented 3 years ago

Maybe you can issue a warning at the start of the launch of the app if the user does not have write access to the CRON_LIVE folder.

hedsnz commented 3 years ago

How does that look? We test for write permissions as well as existence of the path, every one second after input$rscript_repository changes.

The other thing I realized is that I've changed the README.md file directly, but it looks like that's just the output of the cronR.Rmd vignette. Would it be better for me to make those changes to the vignette, and copy the output over to the README? Happy to do this and squash all the commits into a single commit for a new PR if that's helpful.

jwijffels commented 3 years ago

Was on holiday. That looks ok, only that this increases the version of shiny to a >= 1.0.0 which is needed to use debounce. I personally have a lot of servers where I don't want to upgrade shiny and all of it's dependencies for this debounce feature.