845 path/pkg arg consistency

[ilyaZar]

fixes #845

Maybe path is a bit more general because it can be used for files and package-dirs, but the issue suggests pkg.

I have made a commit per file to identify the instances so if required one can quickly change from pkg back to path.

DONE: path - > pkg changes

• R/golem-yaml-get.R
• R/config.R
• R/golem-yaml-set.R
• R/is_golem.R
• R/pkg_tools.R

TO-DO: decide here I do not think pkg makes sense because:

1. sometimes path is a path to a file so pkg is just weird:
   - utils.R - > create_if_needed()-dance for files
   - use_file.R - > template files
   - use_favicon.R - > path to favicon files
   - templates.R - > all functions refer to template files I think
   - modules_fn.R - > module_template() it does use a path to a file
   - add_r_files.R - > append_roxygen_comment() path to the R script where the module will be
   - bundle_resources.R - > bundle_resources() needs a path to a subdir, not the package
   - config.R - > everything but the last function attempts to find a config-file trying different paths
   - create_golem.R - > creates a golem in the specified path and the package_name can be seperately
   - enable_roxygenize -> path to .Rproj file

2. for bootstrapped functions, same as above, plus their args are already named path, I wouldn’t want to change that:

   • bootstrap_fs.R
   • bootstrap_attachment.R
   • bootstrap_dockerfiler.R
   • bootstrap_pkgload.R
   • bootstrap_usethis.R

[ColinFay]

Thanks a lot @ilyaZar, I'll cherry pick this and merge.

You're right about not changing some of the args, as far as I can tell the idea is to have a consistency in user-facing functions where we need to refer to a path to a working directory.

And all things considered, I'm thinking about switching to an arg called golem_wd, so that it's clear what we are talking about :)

[ilyaZar]

@ColinFay allright thanks! let me know which of the commits you'd like to cherry pick (i.e. which of the files basically).

and when you decide on golem_wd or another name, I can make changes to my branch (current pull of upstream dev plus name changes to golem_wd) or squash/separate commits if it helps with the merge

cheers

[ColinFay]

Hey,

It's been too long since this PR has been open, I'm very sorry I didn't pick it up sooner.

I've given it a lot of thoughts overt the last couple of days and by looking at the code, I feel like the functions should behave as is:

• Every fun that relies on the current working directory (aka the ones that evaluate to golem::get_golem_wd() ) should take an arg called golem_wd, (well, except get_golem_wd() itself)
• Every fun that point to the path to a file can be kept as path (as we're referring to the path to a file)
• No bootstrap function should be changed, we need to keep the default arg name

So, I'll cherry pick the commits and do the implementation, even more given that we need to signale this deprecation to the users: an argument that disappears without warning is a breaking change so we would want to keep it alive for some time so that the user have time to change. That being said, I suspect that most of the functions that will see their arg name change are less used functions, and should not be used in the app so we should be safe.

Here is the pattern I'll be implementing :

# Start with a wrapper fun to signal deprecation 

signal_path_is_deprecated <- function(
  path,
  fun,
  golem_wd
) {
  if (!rlang::is_missing(path)) {
    warning(
      sprintf(
        "The `path` argument of `%s()` is deprecated\nand will be removed in a future version of {golem}. \nPlease use the `golem_wd` argument instead.\nNote that the current call to `%s()` will not \nuse the value from `path` and will read the value\nfrom `golem_wd`.",
        as.character(fun),
        as.character(fun)
      ),
      call. = FALSE
    )
  }
}

Then in the funs:

#' @importFrom config get
get_golem_things <- function(
  value,
  config = Sys.getenv(
    "GOLEM_CONFIG_ACTIVE",
    Sys.getenv(
      "R_CONFIG_ACTIVE",
      "default"
    )
  ),
  use_parent = TRUE,
  golem_wd,
  path
) {
  signal_path_is_deprecated(
    path,
    fun = as.character(sys.call()[[1]])
  )

Which will result in:

> get_golem_things(path = ".")
Warning message:
The `path` argument of `get_golem_things()` is deprecated
and will be removed in a future version of {golem}. 
Please use the `golem_wd` argument instead.
Note that the current call to `get_golem_things()` will not 
use the value from `path` and will read the value
from `golem_wd`.

Also, there needs to be some internal rework of the functions / tests that rely on these functions so that we're sure nothing breaks :)

Thanks again for going over all the cases in the package, that's a huge, huge help.

[ColinFay]

Hey,

Your commits have been cherry-picked (so that we still keep your contribution) and reworked in https://github.com/ThinkR-open/golem/tree/845-cherry-pick-path-to-pkg

Thanks again for all your contribution to golem 🫶