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

Feature Request: Function Callbacks on Options #5

Open
billdenney opened this issue Jul 2, 2018 · 10 comments
Open

Feature Request: Function Callbacks on Options #5

billdenney opened this issue Jul 2, 2018 · 10 comments

Comments

@billdenney
Copy link
Contributor

I have written an (unfortunately) option handler within the PKNCA package due to necessity and not really looking around for a better solution as I was doing it. I'd like to move the option-handling functions out of PKNCA (billdenney/pknca#71) so that the PKNCA package is focused on its real intent and the option handling is done by a more appropriate package.

With a first look, the only thing missing from your settings package that I use in PKNCA is the ability to have a function callback that checks, modifies, and sets options.

Would you be interested in a pull request that adds the ability to have function callbacks for option setting? (Or, did I miss the feature in the documentation?)

@markvanderloo
Copy link
Owner

markvanderloo commented Jul 2, 2018

I'm not sure what you mean by 'function callback'. Could you give an example of the workflow you'd like to see?

In the vignette you can see that it is possible to define options managers that restrict values allowed for the user, see under limiting options (I think the vignette could have been better organized, now it starts with a fairly unimportant feature).

@billdenney
Copy link
Contributor Author

What you have under limiting options appears to be very close to what I'm wanting. I was hoping for the additional ability to transform the option to standardize it. A simple example would be so that the option must always be lower case (my real case is more complex). What I'd hope for is something like the example given with inlist where it makes the value lower case like:

library(settings)

inlist_lower <- function (...) {
  .list <- unlist(list(...))
  function(x) {
    x <- tolower(x)
    if (!x %in% .list) {
      stop(sprintf("Option value out of range. Allowed values are %s", 
                   paste(.list, collapse = ",")), call. = FALSE)
    }
    x
  }
}

opt <- options_manager(foo="up", bar=2,
                       .allowed = list(
                         foo = inlist_lower("up","down"),
                         bar = inrange(min=0, max=3)
                       )
)

opt(foo="UP")
opt()
# I'd like opt$foo == "up", but opt$foo == "UP"

Essentially, if the .allowed function returns a value, use that value as the option value.

My real example has multi-column checks on a data.frame, and if an incomplete data.frame is provided, then the missing columns are added.

@markvanderloo
Copy link
Owner

So, I think it now does essentially what you want:

# function with some hard-coded checks
# x: the option value that will be passed by the options manager.
my_checker <- function(x){
  x <- tolower(x)
  if (! x %in% c("bla","yada")){
    stop("Red Alert. Red Alert!",call.=FALSE)
  }
  x
}

# define an options manager
my_options <- options_manager(foo="bla"
      , .allowed=list(foo=my_checker))

Let's see the output:

my_options(foo="BLA")
my_options("foo")
[1] "bla"

Would be grateful if you could give it a whirl. I need to document this, probably in a small 2nd vignette and then it can go to CRAN.

To test, you need to clone and build it (my drat repo is fubar'd somehow because I played with some GH settings). Current GH version is 0.2.5.

@billdenney
Copy link
Contributor Author

That looks good. I haven’t had a chance to test it yet, but I’ll try to in the next few days.

The first thing that occurs to me for testing is, how will it handle an option that is supposed to actually be NULL?

@markvanderloo
Copy link
Owner

should work, especially since latest commit.

@markvanderloo
Copy link
Owner

I think the updated checking API is realy useful, but I wonder if you really want to store whole data.frame in an options field. In my idea, options are basically like environment variables: numbers or short strings that influence the behaviour of a programme. If its a data frame, it may as well be data and thus an argument to a function?

@billdenney
Copy link
Contributor Author

For the reason to store a whole data.frame, it's a default setting for how to perform many calculations as a coherent set. Most of the options I'm using are scalars. It controls the default method for doing calculations when a single dose of a drug is administered.

On to the checking...

@billdenney
Copy link
Contributor Author

Here are some notes on checking it:

Check 1

The change appears to break the prior behavior of the .allowed argument (I think-- I didn't use the package previously, so I don't know).

library(settings)                        
                                         
my_checker <- function(x){               
x <- tolower(x)                          
if (! x %in% c("bla","yada")){           
stop("Red Alert. Red Alert!",call.=FALSE)
}                                        
}                                        
                                         
my_options <- options_manager(foo="bla", 
.allowed=list(foo=my_checker))           
my_options(foo="BLA")                    
#> Error in .op[vars] <<- L: replacement has length zero
my_options("foo")                        
#> [1] "bla"
my_options(foo="yada")                   
#> Error in .op[vars] <- L: replacement has length zero
my_options("foo")                        
#> [1] "bla"
my_options(foo="bing")                   
#> Error: Red Alert. Red Alert!
my_options("foo")                        
#> [1] "bla"

To restore the prior behavior would be more complex because you'd have to treat a NULL return (what my_checker does now) as "accept this value" and anything else as "set the value to this". To set an actual option to NULL would require list(NULL) output. It may be preferable to have a different option than the current .allowed (or to have all your reverse dependencies update their code if they use .allowed).

Check 2

When initially setting an option with the .allowed argument, it should be checked the first time it's set (when initially setting up the options) to ensure that it's consistent:

library(settings)                        
                                         
my_checker <- function(x){               
x <- tolower(x)                          
if (! x %in% c("bla","yada")){           
stop("Red Alert. Red Alert!",call.=FALSE)
}                                        
x                                        
}                                        
                                         
my_options <- options_manager(foo="BLA", 
.allowed=list(foo=my_checker))           
my_options("foo")                        
#> [1] "BLA"
my_options(foo="BLA")                    
my_options("foo")                        
#> [1] "bla"

(Note that the first time when option_manager is called the value is set to something that should not be valid.)

Check 3

An option that doesn't exist looks the same as an option set to NULL. I would hope that the my_options("bar") call at the bottom of this example would be an error rather than NULL.

library(settings)                        
                                         
my_checker <- function(x){               
if (!is.null(x)) {                       
x <- tolower(x)                          
if (! (x %in% c("bla","yada"))) {        
stop("Red Alert. Red Alert!",call.=FALSE)
}                                        
}                                        
x                                        
}                                        
                                         
my_options <- options_manager(foo=NULL,  
.allowed=list(foo=my_checker))           
my_options("bar")                        
#> NULL

@billdenney
Copy link
Contributor Author

billdenney commented Jul 7, 2018

For Check 3 above, I think that something like the following would need to be inserted just above this line:

return( if (length(vars)==1) .op[[vars]] else .op[vars] )

missing_vars <- vars %in% names(.op)
if (length(missing_vars) {
  stop("Cannot find option(s): ", paste(names(.op)[missing_vars], collapse=", "))
}

@markvanderloo
Copy link
Owner

Thanks Bill, I haven't had a chance to take a good look at this for a while. I'll try to make some time so we can fix everything and try to release next Friday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants