vincentarelbundock · GitHub

Thanks for this @vincentarelbundock... I finallly had time to review.

I've let a quite a few comments. Some of them are nits, some are clarifications, and some are a suggestions for potential improvements.

I have two high-level observations:

  1. This container approach is cool, but I think we can tighten up on consistency and (hopefully) efficiency. One concern I have is the fact that we're basically lugging this settings item throughout the internal code, but "manually" reassigning certain elements each time, e.g. settings = update_settings(settings, list(x = x)). In that sense, it's really a glorified wrapper around modifyList (which is fine!). But perhaps we could use the fact that the target is always the same---i.e, settings---to tighten up the implementation so that the code becomes update_settings(x = x)? I don't know whether we should enable update by reference, but it could simplify the code further.

  2. Following on from pt. 1, I'm a little uncomfortable with the fact that many of our internal functions now collapse down to taking a single settings argument, without explicitly making clear which settings are going to be extracted or adjusted. (I mean, you can obviously look inside the relevant function, but everything is more hidden.) I don't have an obviously better solution, but perhaps we can tweak the implementation so that instead of, say,

data_abline = function(settings, ...) {
    list2env(settings[c("datapoints", "lwd", "lty", "col")], environment())
...

we end up with

data_abline = function(params = c("datapoints", "lwd", "lty", "col"), ...) {
    list2env(settings[params], environment())
...

Or, if you wanted something fancier, you could extract the function args as strings:

data_abline = function(datapoints, lwd, lty, col), ...) {
    call = match.call()
    param_names = setdiff(names(formals()), "...")
    params = sapply(call[param_names], deparse1)
    list2env(settings[params], environment())
...

Read the original on github.com ↗