r-lib / devtools

Tools to make an R developer's life easier
https://devtools.r-lib.org
Other
2.37k stars 753 forks source link

Function factories defined out of collation order lead to head-scratching errors. Adding a hint would help. #2451

Closed wdkrnls closed 1 year ago

wdkrnls commented 1 year ago

Is your feature request related to a problem? Please describe.

I was trying to figure out why a function make_function defined in file b.R which defines another function cool_function in file a.R wasn't being seen by devtools::document() resulting in pkgload::load_code("a.R") complaining that it ! could not find function "make_function" .

a.R contained:

offer_scone = make_desert("scone")

b.R contained:

make_desert = function(kind = c("pie", "cake", "scone")) {
  kind <- match.arg(kind)
  unit = switch(kind,
                pie = "slice",
                cake = "piece",
                scone = "bit")

  function(serving) {
    stopifnot(length(serving) == 1)
    if(serving == 1) {
      message("Here is a ", unit, " of ", kind)
    } else {
      message("Here are ", serving, " servings of ", kind)
    }
  }
}

When I ran devtools::document() I saw:

Error in `load_all()`:
! Failed to load R/a.R
Caused by error in `make_desert()`:
! could not find function "make_desert"

I ran a similar experiment where I dropped the higher order function part of this, but found the same issue.

Eventually after a day of digging through documentation I figured out this was because my package collation order was wrong and that roxygen already was smart enough to fix that with an additional @include directive.

Describe the solution you'd like

I would like to be able ignore collation altogether and place code in files with names I find to be descriptive without having to worry about name ordering. R should just be able to "figure it out". However, since this is out of scope for this package I probably will have to settle for a more informative error message.

It would be nice if devtools could give a special hint like below:

Error in `load_all()`:
! Failed to load R/a.R
Caused by error in `make_desert()`:
! could not find function "make_desert"
Hint: Try adding @include b.R to the roxygen documentation for this function call to fix the file collation order.

As an aside, this is what it says right now on the R Packages (2e) website:

Collate controls the order in which R files are sourced. This only matters if your code has side-effects; most commonly because you’re using S4.

It would be nice if that was extended if only with a link to also reference function factories and the @include directive.

wdkrnls commented 1 year ago

A similar issue seems to pertain to naively trying to define an R6Class in your project. However, since this function is in another package, it's not clear how to fix it. You can't just put #' @importFrom R6 R6Class on top of your class definition. Double colons work though, which is what you see when you search for examples in the roxygen documentation or on StackOverflow.

hadley commented 1 year ago

I don't think there's an easy way to automatically detect this — to figure out that make_desert was defined in other file, we'd need to parse every other file to detect that it was an out-of-order definition rather than a typo. Unfortunately I think the effort required to do this out weighs the utility. Even though I'm closing this issue, I really appreciate the feedback, and hope you'll continue to contribute in the future 😄