Issues with positional arguments and shiny.express.ui components

[cpsievert]

Problem

I can see two issues with how shiny.express.ui handles positional arguments:

1. Many functions don't pass along positional arguments (i.e., they have * instead of *args in their signature). This means you can't currently use things like ui.card() the "functional" way:

from shiny.express import ui

ui.card("foo", "bar")

TypeError: card() takes 0 positional arguments but 2 were given

2. Some functions have named positional arguments, which forces those arguments to be specified when used the "context-manager" way. For example, since value_box() has title/value as named positional args, usage must look like this:

with value_box("Title", "Value"):
    pass

...which is unfortunate, especially when you want to a dynamically rendered value.

Proposal

I think we can solve both issues by taking inspiration from how card_header() currently works. That is, if the underlying UI function takes any positional arguments, we always start the express function with *args and pass those args to the underlying UI function

py-shiny/shiny/express/ui/_cm_components.py

Lines 455 to 487 in b61452d

	def card_header(
	*args: TagChild | TagAttrs,
	container: TagFunction = ui.tags.div,
	**kwargs: TagAttrValue,
	) -> RecallContextManager[CardItem]:
	"""
	Context manager for a card header container
	This function wraps :func:`~shiny.ui.card_header`.
	A general container for the "header" of a :func:`~shiny.ui.card`. This component is designed
	to be provided as a direct child to :func:`~shiny.ui.card`.
	The header has a different background color and border than the rest of the card.
	Parameters
	----------
	*args
	Contents to the header container. Or tag attributes that are supplied to the
	resolved :class:`~htmltools.Tag` object.
	container
	Method for the returned Tag object. Defaults to :func:`~shiny.ui.tags.div`.
	**kwargs
	Additional HTML attributes for the returned Tag.
	"""
	return RecallContextManager(
	ui.card_header,
	args=args,
	kwargs=dict(
	container=container,
	**kwargs,
	),
	)

This does lead to the question of whether functions like nav_panel() should be (title, *args, **kwargs) or (*args, **kwargs), but it seems pretty sensible to me (for consistency sake) that we go in the latter direction.

[jcheng5]

1. I thought we decided before the break that we weren't going to allow shiny.express.ui.card to take children the functional way? The exception to this being simple HTML tags, and even then, we wouldn't encourage mixing.
2. I think we were thinking of that as a feature, not a bug--things that feel like parameters are passed the functional way, while things that feel like children are passed as child expressions to the context manager. (I acknowledge that this makes dynamically rendered values harder.)

Even if the above were not true, I don't love--if I understand correctly--that functions won't be able to take non-child positional arguments anymore. In the nav_panel example, it means that the title= would have to come later in the source code than the children, when using Shiny Core.

[gshotwell]

My apologies if this is off base, but would it be helpful to maybe write a document expressing the line between Core and Express? I feel like some of these API decisions might be getting a bit muddy because as we're trying to do a bit too much with Express layouts. We're going to need that documentation anyway to tell people when they should switch to Core, and I think it would clarify some of these questions because we'd have more consensus on what we need to make easy in Express and what we can allow to be a bit more difficult.

[cpsievert]

I think we were thinking of (1) as a feature, not a bug.

I'd be curious to hear more of the reasoning behind this. From my limited understanding, it seems these costs outweigh the benefits:

1. It'll be confusing/frustrating to learn/remember why ui.input_action_button("foo", "bar") works, but ui.card("foo", "bar") doesn't.
2. Having code like ui.card("foo", "bar") work in Express makes it easier to transition to a Core app.

If we do, however, decide that erroring is desirable, seems we could improve the error message.

Also, I think the value_box() thing (problem 2) is a much higher priority, so I created another PR (#966) that addresses that problem (without allowing positional args passed to the context manager)

would it be helpful to maybe write a document expressing the line between Core and Express?

Not necessarily for solving this problem, but I do think that'll be helpful to do at some point. I would also be surprised if we have a clear vision for that line at this point :)

[cpsievert]

Update: the value_box() issue was addressed in #966 and I also added a few more thoughts on why I think it makes sense to accept positional args in #950