Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

845 path/pkg arg consistency #1048

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

ilyaZar
Copy link
Contributor

@ilyaZar ilyaZar commented May 26, 2023

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

@ilyaZar ilyaZar mentioned this pull request Aug 9, 2023
@ColinFay
Copy link
Member

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
Copy link
Contributor Author

ilyaZar commented Nov 24, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants