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:
-
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
settingsitem 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 aroundmodifyList(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 becomesupdate_settings(x = x)? I don't know whether we should enable update by reference, but it could simplify the code further. -
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
settingsargument, 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()) ...