klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
862 stars 48 forks source link

Documentation not found for reexports #330

Open owenjonesuob opened 1 year ago

owenjonesuob commented 1 year ago

Error description

If a module imports, and then re-exports, an object from another module, then box::help() doesn't look for documentation in the right place.

We can see this with ./bio from the "Nesting modules" section of the "Getting started" vignette. If we implement bio using the "reexport" pattern at the end of that section:

#' @export
box::use(./seq[...])

Then we fail to retrieve help for is_valid(), because box::help() is looking for documentation in bio rather than in seq:

box::use(./bio)
box::help(bio$is_valid)
#> Error in box::help(bio$is_valid) : 
#>  no documentation available for “is_valid” in module “./bio”

R version

4.3.1

‘box’ version

1.1.3 (from CRAN), build/HEAD (from GitHub)

klmr commented 1 year ago

Thanks for the report, I can confirm that this is a bug. Unfortunately I am not sure that it will be easy to fix, since the way reexports are currently handled loses all information about them being reexports.

(Note to self:)

I’ll probably have to change the return type of parse_export_specs to include information about where a given name is reexported from (if it is a reexport), maybe via an attribute. Then I’ll need to add that information to the namespace information in load_from_source. And then I’ll need to adapt box::help to select the correct namespace.

owenjonesuob commented 1 year ago

I'm afraid I only managed to dig a little way into this myself before getting stuck - I will share my only finding in case it is helpful, though I suspect you might have a better way internally of getting the same info!

With the same example from above:

attr(attr(bio$is_valid, "srcref"), "srcfile")
#> /path/to/box/vignettes/bio/seq.r

From some quick ad hoc testing, this seems to point to the right file (i.e. the file where the function is defined) even if we re-export a function all the way up a stack of modules.

klmr commented 1 year ago

Hmm. This is actually more complex than I thought.

Consider the following module a.r:

#' @export
box::use(./other_mod[alias = name])

If a user now imports a and runs box::help(a$alias), they want to be shown the help of name from a/other_module.r. But that help document will use name instead of alias everywhere, which would confuse the user (consider reading the documentation for the function foo$bar() and bar is never mentioned, but instead the documentation mentions how to use frob).

Not sure what a good solution for this is, except to force users to provide new documentation for documents that are re-exported under an alias. And annotating reeexports with ‘roxygen2’ isn’t supported yet (and there are issues with that as well; such as enforcing that documentation is applied to a reexport that names a single object).

klmr commented 1 month ago

(Note to self:) also asked about on Stack Overflow.