Closed toph-allen closed 1 month ago
I'm not sure why, but this PR has been sitting unreviewed for almost a month. The connectapi
release will happen imminently, and I'd either like to merge this or put it on hold.
I don't like the implementation as written. Currently it requires a content item and variant key to render a variant, but in connectapi
's currently schema, it should really also accept a variant
. However, variants also have the render()
method to render them; this seems duplicative.
In the future, we're also going to move away from the Variant
class being an equivalent to Content
.
The real trouble I'm having making this fit into the current type system of the package is just around the return value from this function. I was going to push an update to return a VariantTask
when the object rendered is a variant, and a ContentTask
when it's the default variant, but then, what if they pass in a content item and a variant key? Return a VariantTask
, obviously. What if they specify the default variant with a variant_key
or pass in a Variant
class object representing the default variant?
Even more confusing because VariantTask
inherits from Variant
and ContentTask
inherits from Content
— it's like the current type system is trying to have it both ways and return both a task object and the object it was passed, kinda hacking in multiple inheritance even though R6 doesn't support it.
This PR adds a new argument to
content_render()
,variant_key
, which isNULL
by default. If a value is passed in, this is used to look up a non-default variant to render. By default, if no value is provided forvariant_key
, the default variant is rendered.content_render
now returns aVariantTask
object, not aContentTask
.ContentTask
inherited fromContent
, andVariantTask
inherits fromVariant
, which inherits fromContent
. So, it's basically aContent
object withVariant
data and aTask
subfield.get_variant()
. Before, you had to pass in the magic string"default"
tokey
. I tried changing the default tokey = NULL
, and having that return the default variant. I wound up rolling that change back to how it was before, because it strikes me as a little weird thatget_variant(content)
, with no arguments, returns the default variant.