config collection
Opened this issue · 5 comments
The config element can appear only at the top-level of a spec, if concatting two specs with configs, they should be blended.
Perhaps a warning if there are conflicts.
Based on schema I think there are a number of 'top level' keys that aren't part of 'inner' specs:
TOP_LEVEL_KEYS <- c("$schema", "autosize", "background", "config", "datasets",
"padding", "usermeta")In vlbuildr I have used those to separate out 'inner' and 'outer' specs (code) to power some preliminary concatenation functions.
The approach is fairly crude in combining after that (just uses Reduce + modifyList for combining the outer specs, which could easily lead to invalid specs)... I wanted to get to a v0 of the composition functions but am hoping to later refactor to just call the functions in this package 😄 (e.g. vegawidget/vegabrite#8)
I think we are thinking along the same sort of lines here - that we would need a set of rules to manage conflicts. Here's my gut-level reaction for each of the TOP_LEVEL_KEYS - I'd be interested in your reactions (I also need to read up on Reduce and modifyList, I have been spoiled by purrr):
default rule: favor "something" over "nothing"; if there is more than one "something", pick the first one - this might be what you are doing already.
$schema- they should not disagree, if they do disagree, revert to major-version. If disagreement on major-version, or if one is Vega and the other is Vega-Lite, we can't do anything for you.autosize- not supported for concatenated specs, might be best to discardbackground- use default ruleconfig- use default rule (might have to be careful here with things like colormaps)datasets- I propose to combine datasets, rename where needed, and update the references in the constituent specspadding- use default ruleusermeta- does not affect the rendering, we could just combine lists into bigger lists.
I am happy to make a try at something like this, in consultation with you on the "rules". 😃
Yes we're thinking along the same lines!
What modifyList(x,y) does is keep any elements of x that are not in y, add in new elements from y, and replace elements in x that are in y with the y version -- but do that recursively in case the element is a list.
So in this example:
list1 <- list(a = 1, b = 2, d = list(e = 3, f = 4))
list2 <- list(b = 5, d = list(g = 6), h = 7)
modifyList(list1, list2)b from list1 gets replaced by the value from list2
h from list2 gets added to the list
a stays the same because it isn't in list2
and d gets modified recursively via modifyList -- so g from list2$d gets added to list1$d
Reduce was to do that iteratively across potentially more than two lists.
One problem with the modifyList approach is I don't think we necessarily want to do that kind of merging recursively, as that could lead to invalid objects. However, I do think in some cases one level of recursion might make sense. E.g. for config if two of the specs have a config defined, but one of them has a mark config defined while the other has a legend config I think it would make sense to make a config that has both the mark and the legend config option in config. But if both have a mark config defined, only pick one of them because we don't want to worry about potentially mutually exclusive properties within the mark config option.
I think what I'm suggesting is a slight modification to your proposed default rule -- for that rule to also have an option of how many levels to recurse when being applied to a particular top level spec. For something like config I think it would be 1 level, for usermeta it might be unlimited, and for others it might be no recursion.
just chiming in here to add that maybe it's worth formalising each part of the spec as an S3 or S4 class, then defining methods for combining them. Seems to me that approach would deal with the edge cases you've mentioned.
Hi @sa-lee, agreed! I think using S3 methods and classes would serve us well both here and in ggspec (cc @haleyjeppson).