ThinkR-open / golem

A Framework for Building Robust Shiny Apps
https://thinkr-open.github.io/golem/
Other
908 stars 132 forks source link

819 - refactor use_files.R before adding replace arg #1022

Closed ilyaZar closed 1 month ago

ilyaZar commented 1 year ago

To fix #819 , there is the possibility to refactor the internals of use_external_XXX type functions in use_files.R prior to the addition of a new replace arg.

This may or may not be useful; I think it could be, hence the draft PR with some benefits being to reduce duplication

It's easier to add the 'replace'-argument in only one place then (see last commit to this PR).

Essentially all use_external_xxx funcs in use_files.R do the same, so this PR tries to encapsulate identical behaviour into smaller maintainable pieces.

Of course this proposal comes along with quite some work and probably there should be some tests added as well... happy do get on this though!

VincentGuyader commented 1 year ago

HI,

i prefer overwrite = TRUE, instead of replace = TRUE. ok for you @ColinFay ?

ilyaZar commented 10 months ago

Update

Update this PR-branch to be up-to-date with current dev:

  1. change replace-argument name to overwrite
  2. use the following style https://github.com/ThinkR-open/golem/commit/7ba820b3693753d99a58fb309e4278db7e326194#diff-c03aab5bb43f6562efff9eeb973b449f684e38dd79aed2b839008ffaeee45a8eR183-R194 for get_new_dir() helper function to throw an error
  3. use check_name_length_is_one instead of check_name_length()
  4. set default value NULL for argument name

Notes:

@ColinFay this PR is a bit of a mess unfortunately as it had to be adjusted to updates made in https://github.com/ThinkR-open/golem/commit/7ba820b3693753d99a58fb309e4278db7e326194

But since there is a 100% test coverage for the file R/use_files.R (that has been introduced back then in commit https://github.com/ThinkR-open/golem/commit/7ba820b3693753d99a58fb309e4278db7e326194 ) it should be rather safe

Let me know if you request any other changes !

ColinFay commented 1 month ago

Hey,

Thanks a lot for your contribution (as usual, you rock 🤘).

I've worked on a full refactoring of the use_* family that you can find here : https://github.com/ThinkR-open/golem/pull/1170

I've gone way deeper into factoring the function so that it's more consistent, and it will make it easier to add new use_* functions if we wish to.

Closing this one, but hanks again 🙏