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

Rigging more than one function #16

Closed
krlmlr opened this issue Apr 26, 2021 · 16 comments
Closed

Rigging more than one function #16

krlmlr opened this issue Apr 26, 2021 · 16 comments
Milestone

Comments

@krlmlr
Copy link
Collaborator

krlmlr commented Apr 26, 2021

that call each other.

#11 (comment)

I'd like to propose a wire() function:

wire <- function(..., ignore = , ) {
  names <- c(...)

  # rigs all functions
  # ensures that the function environments are updated with the newly rigged functions
}

And also ignite(), to be called from .onLoad():

ignite <- function() {
  config <- Sys.getenv("R_BOOMER")
  ...
  wire(...)  
}

To be used like this:

.onLoad <- function(...) {
  if (requireNamespace("boomer", quietly = TRUE)) {
    boomer::ignite()
  }
}
@moodymudskipper
Copy link
Owner

wire rigs in place and thus can take several inputs ?
I could see how it could be handy, I could trace the functions to return the output of a call to a rigged copy, and we'd use untrace to "unwire".

I don't understand what ignite() does, does it rig all the package's functions ?

The use case here is package development ?

I think for wire, if I understood correctly what it should be, I'd use a more boring name such as trace_rig or rig_in_place to make the difference between functions more obvious.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 26, 2021

Yes.

ignite() takes inspiration from how {debugme} handles this: you can specify in an environment variable which functions to rig, and don't need to change the code to enable or disable rigging. This allows rigging an installed package too.

I wouldn't couple this too tightly with trace(). Also, since this is for package development, there's no need to support un-rig -- a fresh R session without the environment variable does the job.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 26, 2021

How about multi_rig() or rig_some() instead of wire()?

@moodymudskipper
Copy link
Owner

Thanks, I'll take a look at {debug me}, sounds interesting

@moodymudskipper
Copy link
Owner

moodymudskipper commented Apr 27, 2021

If we modify functions in their namespace :

If we use devtools, I believe we can simply place rigged copies in an environment attached on the search path right next to "devtools_shims", let's call it "boomer_shims".

  • it is less weird
  • we can rig all functions
  • building helpers to unrig, or have multi_rig work like dplyr::select by doing multi_rig(-this_fun, -that_fun)
    This way we can also rig functions from other packages, including base.

The rigged function's body's functions should be evaluated first in "boomer_shims" and then in the rigged function's environment, so it can work with functions calling each other, I think we can use :

shim_list <- as.list(as.environment("boomer_shims"))
ns <- parent.environment(environment())
eval(fun_sym, shim_list , ns)

Rigging namespaced functions would be harder, e.g. if i want to multi_rig(dplyr::filter).

To support it we might :

  • have multirig(dplyr::filter) add a function called "dplyr::filter" to "boomer_shims" (non syntactic, highly unlikely to conflict in practice, and easy to understand)
  • override :: and ::: in boomer_shims so that they look for a match in "boomer_shims" before checking the actual namespaces.

It's still not quite enough because if I've attached dplyr or do it later, filter should find a shimmed copy in "boomer_shims" ,
so multirig(dplyr::filter) should also create a copy of filter, but this copy should check if {dplyr} is attached and do the appropriate action (rigged dplyr::filter or simple stats::filter, explicit failure for cases where nothing on the search path).

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 28, 2021

This starts getting very complicated. The current implementation works great. I suspect shims might get us into other kinds of trouble down the line. I remember this painfully from mockr. Also, I don't care too much about unrigging.

I see two components of the problem:

  1. Functions a() and b() a rigged, if a() calls b() it uses the original version of b() (or the other way round). I think you confirmed that?
  2. A way to conveniently rig functions in your own package without changing the code.

The first can be achieved with a simple extension of rig() AFAICT: get all functions to be modified, rig them, update the environments for all functions, update them. Assigning to the own namespace during loading shouldn't bother CRAN too much.

For the second problem, {debugme} can provide inspiration.

@moodymudskipper
Copy link
Owner

moodymudskipper commented May 4, 2021

@krlmlr I've implemented rig_in_namespace

I've played around a bit and it seems to work fine,

it's a bit hard to follow the log though, maybe the rigged function should announce itself : we'd print in a different color "going through foo(...)", "going through bar(...)", "back to foo(...)". It would change the body though, so wouldn't be robust in some corner cases such as displaying a function's body.

Another option is to change colors each time we enter a different rigged function, 3 cycling colors would be enough for us to know if we're going forward or back in the stack.

unexported functions when not using devtools::load_all() (which basically exports them), is not well supported yet.

I'd like a new rig_in_namespace call to cancel the previous one, OR to add to it, depending on a parameter. For this we need to be able to unrig, but it'll be possible through #23

I think rig_in_namespace could work by default a bit like dplyr::select, we can use - to unrig, we could use tidy selection and we'd be able to rig what starts_with() or ends_with() something for instance (or use everything()). an argument .keep = FALSE triggers whether we start from already rigged functions or from scratch.

@krlmlr
Copy link
Collaborator Author

krlmlr commented May 25, 2021

To unrig, can we store the old function code?

rig_in_namespace() doesn't yet work for me in .onLoad(). Should it?

@moodymudskipper
Copy link
Owner

To unrig, can we store the old function code?

The code doesn't change, the environment does, and we'd need to store it indeed

@krlmlr
Copy link
Collaborator Author

krlmlr commented Jun 7, 2021

Most functions use the package environment, but I agree it's safer to cache it for the general case.

@moodymudskipper moodymudskipper added this to the CRAN (0.1) milestone Jun 9, 2021
@krlmlr
Copy link
Collaborator Author

krlmlr commented Jun 11, 2021

Currently, it seems difficult to create a reprex with rig_in_namespace() . What can we do about that?

@moodymudskipper
Copy link
Owner

moodymudskipper commented Jun 15, 2021

This seems to work :

fake_package <- function(name, exported = NULL, unexported = NULL, attach = TRUE) {
  # fetch and eval call to create `makeNamespace`
  eval(body(loadNamespace)[[c(8, 4, 4)]])
  # create an empty namespace
  ns <- makeNamespace(name)
  # makethis namespace the closure env of our input functions
  exported <- lapply(exported, `environment<-`, ns)
  unexported <- lapply(unexported, `environment<-`, ns)
  # place these in the namespace
  list2env(exported, ns)
  list2env(unexported, ns)
  # export relevant functions
  namespaceExport(ns, names(exported))
  if(attach) {
    # copy exported funs to "package:pkg" envir of the search path
    attach(exported, name = paste0("package:", name))
  }
  invisible()
}


fake_package(
  "fake", 
  exported = list(add_2 = function(x) add_1(x) + 1, add_1 = function(x) x + 1))

boomer::rig_in_namespace(add_1, add_2)

add_2(5)

image

I will add fake_package to boomer as an unexported function, so we can use it easily. It should probably be in its own package though.

@moodymudskipper
Copy link
Owner

done in 9010b21

In the above I exported both functions, it's not a limitation of fake_package() but of rig_in_namespace(), for now it is not able to rig unexported functions (except when using devtools::load_all(), which basically exports all).

@krlmlr
Copy link
Collaborator Author

krlmlr commented Jun 17, 2021

Could rig_in_namespace() also work for plain environments such as .GlobalEnv ?

@moodymudskipper
Copy link
Owner

The main feature is there, splitting the rest into new issues for V 0.2

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants