vegawidget/vlcompose

S3 objects for different types of spec? Or helpers to determine the type of spec?

Closed this issue · 8 comments

AliciaSchep commented

Hi @ijlyttle not sure the best place to kick off a discussion about this question.. could be a vegawidget question!

I've been thinking for the spec-building package I'm working on that it would be helpful to either have:

(1) separate S3 classes for each type of Spec possible (e.g. FacetSpec, ConcatSpec, RepeatSpec, etc). Objects with these types would also be of class vegaspec. This would enable building S3 classes for various functions (like concatenation, but also other spec modification functions) that are specific to a type of spec.

(2) helper functions to take in a spec and determine what kind of spec it is

What do you think? And would those be functionality in vlcompose, vegawidget, or elsewhere?

ijlyttle commented

For me, this is today's ☕️question. As always, I am happy to propose ideas; I am happier when you react with better ideas 😃

ijlyttle commented

I've kicked this around in my head a bit, to the point where I might have a few half-baked ideas.

In vegawidget, we already have the classes vegaspec, vegaspec_vega_lite and vegaspec_vega. For example:

library("vegawidget")
class(spec_mtcars)
[1] "vegaspec_vega_lite" "vegaspec"           "list"

As a first thought, we could add additional classes in the vegawidget package: vegaspec_facet, vegaspec_repeat and vegaspec_concat that would subclass vegaspec_vega_lite.

I'm thinking vegawidget because we are already looking for compound specs in vw_autosize() - this could be a cleaner implementation than what vw_autosize() does currently.

More thinking out loud, all the vegawidget functions coerce to vegaspecs, so by assigning these sub-classes in as_vegaspec(), this could help make sure that the classes don't get out-of-phase with the spec itself, should it be tweaked outside one of our functions.

Even more thinking out loud, I suppose the methods that would take advantage of this could be internal, e.g.:

vw_autosize <- function(spec, ...) {
  spec <- as_vegaspec(spec)
  .vw_autosize(spec)
}

.vw_autosize.vegaspec_facet <- function(spec, ...) {
  warning("ok, but not gonna do much")
  NextMethod()
}

.vw_autosize.vegaspec_vega_lite <- function(spec, ...) {
  do_something()
}

Would something like this work for what you have in mind? Other ideas?

AliciaSchep commented

Yeah I was thinking of something along those lines in terms of subclasssing vegaspec_vega_lite. Having the as_vegaspec function do this class assignment makes a lot of sense!

ijlyttle commented

I figured we were on the same page on sub-classing 😃

I'll implement this in vegawidget, then close the issue here when done.

Thanks!

  • 👍1
AliciaSchep commented

In terms of the exact classes, I think it would be worth considering having 1:1 mapping to all of the "TopLevelSpec" definitions in the Vega-Lite schema:

VL_SCHEMA$definitions$TopLevelSpec$anyOf %>% purrr::map_chr(~purrr::pluck(., "$ref"))

[1] "#/definitions/TopLevelUnitSpec" "#/definitions/TopLevelFacetSpec"
[3] "#/definitions/TopLevelLayerSpec" "#/definitions/TopLevelRepeatSpec"
[5] "#/definitions/TopLevelConcatSpec" "#/definitions/TopLevelVConcatSpec"
[7] "#/definitions/TopLevelHConcatSpec"

e.g. separate out the different concat versions and also have a class specific to the non-composite spec (TopLevelUnitSpec)

Although for some downstream applications, grouping the concat specs together makes sense; just bringing up the issue in terms of thinking about what the approach will be -- reflecting the exact top level spec types of Vega-Lite or some higher level groupings based on what is likely most practical when working with these specs in R. (Another alternative is to do both... e.g. a hconcat spec gets a class specific to hconcat but also a higher level any_concat class...)

ijlyttle commented

Thanks for digging into this - I agree that it would be a good idea to have a 1:1 mapping, for reasons we (well, I) cannot yet fathom.

If this means a little more complexity for we developers, so be it.

ijlyttle commented

I just realized that agreeing with your proposal puts an extra burden on writing as_vegaspec(). My statement about complexity and developers seemed much more-appealing in the abstract 😳