Public-Health-Scotland / phstemplates

Standard R templates and project structure for Public Health Scotland (https://public-health-scotland.github.io/phstemplates/)
Other
19 stars 6 forks source link

Script template changes and update metadata addin #55

Open alan-y opened 1 year ago

alan-y commented 1 year ago

I made some suggestions. My main concern is that this is quite a large PR so it's a bit tricky to properly review it.

Would you be able to split it into multiple PRs so bits can be checked and merged separately? To that end I'd do something like <checkout new branch from main> then git checkout alan -- <file paths to checkout>. That way you can pull changed files from one branch to another.

I think a lot of the changes occurred after styler was run on it. I think your comments have covered the main stuff that needed reviewed thanks but I also noticed that we should probably try to pick up author name from git config in the shiny app template as well so will take a note to add that in there.

alan-y commented 1 year ago

I left out the use of rlang::check_installed() as I think it doesn't install the package when R is used non-interactively so I think it might not install a package when a user runs the function by opening a new project from the menus.

RosalynLP commented 1 year ago

@alan-y I've been trying to test this and getting issues both using devtools::load_all() from within the project, and when installing the package from the alan branch.

> phstemplates:::new_script()
Error in dyn.load(file, DLLpath = DLLpath, ...) : 
  unable to load shared object '/mnt/homes/rosalp01/R/x86_64-pc-linux-gnu-library/4.1/git2r/libs/git2r.so':
  libgit2.so.26: cannot open shared object file: No such file or directory

This can be traced back to the git2r package which installs fine but when you try to load it gives the error above. Did you come across anything similar when developing?

alan-y commented 1 year ago

This can be traced back to the git2r package which installs fine but when you try to load it gives the error above. Did you come across anything similar when developing?

@RosalynLP - I developed on R Desktop so thanks for trying it out on Posit Workbench. When I get a bit of time, I'll see if there's a way to resolve the git2r issue there but if not, we can always revert to the previous way of handling git in the package.

Moohan commented 1 year ago

I've just resolved conflicts again so I think this PR is as cleaned up as it can be. I guess it's now just on hold until the git2r issue is solved on the workbench.

A possible option would be to pull out the changes not dependent on git2r e.g. most of the changes in R/script_template.R and R/shiny_app_template.R and merge those now. That would be this item from the NEWS.

  • Added Posit Workbench options and quit() to script template.
alan-y commented 1 year ago

A possible option would be to pull out the changes not dependent on git2r e.g. most of the changes in R/script_template.R and R/shiny_app_template.R and merge those now. That would be this item from the NEWS.

Yeah I'd like to tidy up as much as possible and it'd be good to have the changes in script template available for users using POSIT Workbench. I'll put in a separate pull request to deal with that soon.