golem
golem copied to clipboard
819 - refactor use_files.R before adding replace arg
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!
HI,
i prefer overwrite = TRUE
, instead of replace = TRUE
.
ok for you @ColinFay ?
Update
Update this PR-branch to be up-to-date with current dev
:
- change
replace
-argument name tooverwrite
- 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 - use
check_name_length_is_one
instead ofcheck_name_length()
- set default value
NULL
for argumentname
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 !
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 🙏