Closed abhsarma closed 4 years ago
Might need to use .onLoad to set it https://stackoverflow.com/questions/20223601/r-how-to-run-some-code-on-load-of-package
I see. Thanks!
Also related to #58
the .onLoad
implementation: https://github.com/MUCollective/multiverse/blob/6d0e38247df376acc239e4275c4578a765214953/R/zzz.R#L8
allows us to load the engine as well as do the hot-patch for knitting, but that results in warnings RMD check:
Warning: changing locked binding for ‘block_exec’ in ‘knitr’ whilst loading ‘multiverse’
A namespace must be able to be loaded with just the base namespace
loaded: otherwise if the namespace gets loaded by a saved object, the
session will be unable to start.
Probably some imports need to be declared in the NAMESPACE file.
I think part of the problem is that this package imports knitr and not specific functions from knitr, mostly because we need to access the internals of knitr. Not so sure how to fix it?
It's possible you need knitr to be a "Depends" instead of an "Imports" in DESCRIPTION? It's a little annoying in the short term as it will cause it to be auto-attached when multiverse is attached, but eventually (assuming we can get a change into knitr) we won't need it.
See the "good practice" subsection of help(".onLoad")
I think what RMD check is complaining about though is that we are loading knitr
and then changing the binding for a function in its namespace --- it is considered as an unsafe call. So unless there's another way I feel like R will keep complaining about that?
The error in the previous comment goes away when I move the hotpatch code to an .onAttach
function, but basically R is always going to yell if we are reassigning in another packages namespace?
Based on my reading of that error I would expect moving to onAttach to also solve the problem, though then you have the tradeoff that the multiverse package must be attached to use multiverse code blocks (the error is basically saying "you can't modify knitr here because at this point we can't guarantee knitr is even loaded", but I think by the time onAttach happens it can guarantee that). That might be a better compromise than forcing knitr to be attached if you attach the multiverse package though (which is what would happen if you moved knitr to a Depends), so your solution is probably better as long as the docs are very clear that you have to attach the multiverse package (ie use library()) to use multiverse code blocks, eg you couldn't just use M = multiverse::multiverse()
sorry the pervious comment was not very clear. I am still getting warnings because of this: assignInNamespace("block_exec", custom_block_exec, ns = "knitr")
which is basically R yelling that this is an unsafe call...
Found the following possibly unsafe calls: File ‘multiverse/R/zzz.R’: assignInNamespace("block_exec", custom_block_exec, ns = "knitr")
Ah makes sense. In the end not too surprising. I'd say we just have to live with that error for now unless there's another hackish way around it in the short term (possibly assigning assignInNamespace
to another function name then using that, like we did with :::
?).
Ultimately both of those hacks must be removed before a CRAN submission, though they are worth it in the short term to avoid WARNINGs we already know about.
I'd say it's time to file an issue with knitr then. It might be good practice for you to do that (since I did the last one with rstudio? 😛). I'd say something with a minimally reproducible code chunk type and a suggested solution (like marking the return type from a code chunk engine with a class that causes it to skip the post-processing in block_exec).
(we can also draft it together if you'd rather)
When I build the package and install it, then multiverse code block does not seem to work sometimes and I get the following error:
/bin/sh: multiverse: command not found
But the behavior is kind of erratic. This error does not occur when knitting the document