wrong default file path in run_dev() documentation, file path hard coded

[Teebusch]

The run_dev() docstring says that the default path for the project's run_dev.R file is R/run_dev(),

golem/R/run_dev.R

Line 3 in 709a382

#' @param file File path to `run_dev.R`. Defaults to `R/run_dev.R`.

but the default is actually set to dev/run_dev():

golem/R/run_dev.R

Line 14 in 709a382

file <- "dev/run_dev.R"

If the user supplies a file argument, the function checks whether that file exists but then goes on to eval the content of dev/run_dev.R anyway

golem/R/run_dev.R

Line 31 in 709a382

run_dev_lines <- readLines("dev/run_dev.R")

---

I can't make a PR right now or test this, but I think this should fix it (assuming that the default location of run_dev.R is supposed to be in dev):

#' Run run_dev.R
#'
#' @param file File path to `run_dev.R`. Defaults to `dev/run_dev.R`.
#' @param pkg Package name to run the file. Defaults to current active package.
#'
#' @export
#'
#' @return Used for side-effect
run_dev <- function(
  file,
  pkg = pkgload::pkg_name()
) {
  if (missing(file)) {
    file <- "dev/run_dev.R"
  }

  if (pkgload::is_dev_package(pkg)) {
    run_dev_lines <- readLines(
      file.path(
        pkgload::pkg_path(),
        file
      )
    )
  } else {
    try_dev <- file.path(
      pkgload::pkg_path(),
      file
    )

    if (file.exists(try_dev)) {
      run_dev_lines <- readLines(try_dev)
    } else {
      stop("Unable to locate dev file")
    }
  }

  eval(parse(text = run_dev_lines))
}

Let me know if you agree with this solution and I will make a PR.

[ALanguillaume]

Hi @Teebusch,
Thanks a lot for reporting this and fixing our mistakes.
Go ahead with a PR !

[ALanguillaume]

Make sure to start from the dev branch, though, as specified in CONTRIBUTING.md

https://github.com/ThinkR-open/golem/blob/dev/R/run_dev.R

[ColinFay]

Thanks a lot, this is indeed a typo in the documentation.

As far as I can tell from re-reading the code, if you supply another path to a dev file (say, dev/run_dev2.R for example) the function will read this one (because the arg is not missing() anymore), so the behavior is correct here, unless I'm missing something?

That being said, I would suggest moving to a clearer option where the param has the default value file = "dev/run_dev.R".

Let us know if you feel like doing a PR.

Cheers,
Colin

[VincentGuyader]

I'm thinking about something
do we have to set here::here("dev/run_dev.R") to be more defensive ?
(what if someone using fusen launch golem::run_dev() from dev/flat_file.Rmd)

[Teebusch]

As far as I can tell from re-reading the code, if you supply another path to a dev file (say, dev/run_dev2.R for example) the function will read this one (because the arg is not missing() anymore), so the behavior is correct here, unless I'm missing something?

@ColinFay it does use the user-supplied value to build a file path (ln. 25) but then reads the content of dev/run_dev.R anyways, because it is hard coded (ln. 31).

 try_dev <- file.path(
    pkgload::pkg_path(),
    file
  )

  if (file.exists(try_dev)) {
    run_dev_lines <- readLines("dev/run_dev.R")   # this right here
  } else {
    stop("Unable to locate dev file")
  }

[Teebusch]

I'm thinking about something do we have to set here::here("dev/run_dev.R") to be more defensive ? (what if someone using fusen launch golem::run_dev() from dev/flat_file.Rmd)

@VincentGuyader Sounds like a good idea. In addition, the function does not always check if the file exists before trying to read it. Only if is_dev_package() is false. This will throw an error if the file does not actually exist. Is there a good reason to assume that the file will always exist in this case and one can skip the test?

if (pkgload::is_dev_package(pkg)) {
  run_dev_lines <- readLines(    # immediately tries to read file w/o checking existence
    file.path(
      pkgload::pkg_path(),
      file
    )
  )
} else {
# ...
}

[Teebusch]

Here's a suggestion with some additional changes. I'll make a PR later. [Edit: using get_golem_wd(), as seen on the dev branch]

• always check if file exists (nice side effects: removes one if-else)
• use here::here() instead of file.path()
• update docstring title to reflect that the dev file an be called anything, not just "run_dev.R"

#' Run dev file
#'
#' @param file File path to dev file. Defaults to `dev/run_dev.R`.
#' @inheritParams add_module
#'
#' @export
#'
#' @return Used for side-effect
run_dev <- function(
    file = "dev/run_dev.R",
    pkg = get_golem_wd()
) {
  f <- here::here(pkg, file)
    
  if (!file.exists(f)) {
    stop("Unable to locate dev file")
  } else {
    dev_lines <- readLines(f)
    eval(parse(text = dev_lines))
  }
}

[ColinFay]

That sounds awesome 👏

Just a note: there is no need to here::here() the path, as get_golem_wd() is already returning the output of here::here() :)

[odysseyjake]

Hi folks...just noting that this is still an issue in the currently released CRAN version of the package 0.4.1. When is a new version going to be released?

[ColinFay]

@odysseyjake currently working on a new release, so I suppose in november :)

[ColinFay]

That being said, as it is now in the dev branch, I'll close this issue :)