-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Named slots / Nested slots #1145
base: main
Are you sure you want to change the base?
Conversation
Overall, I'm liking the direction of this. Thanks for working on this! My main two suggestions are:
@me.content_component(named_slots=["header", "body"])
def comp():
...
def page():
with comp() as c:
with c.header():
me.text("...")
with c.body():
me.text("...") I'm not totally sure how to write this type annotation / implement this (maybe Gemini can help :), but I think this would provide a much nicer API for consumers for named slot content components because then you can get IDE support (e.g. autocomplete, type-checking) |
I think it's definitely doable to get this syntax, but does not seem like we'd be able to get the dynamic annotations/type hints in IDE in a nicer way.
There is one option which is to use namedtuple
I think this would work, but I think doesn't feel like a good trade off. Since we'd need to create a different namedtuple for each set of named slots. Also wouldn't work if someone did: Another option is for users to implement a dataclass and pass that into the named slots. But seems unnecessarily verbose and probably don't want to expose DetachedNodeTreeStateContext
|
EDIT: Actually NVM, don't think that would work. Hmm, one more idea that could work is to let the user pass in a Literal for the named slots like
Drawback is that this would lead to an api like this:
So not as a nice c.header, c.body |
I think the named slots w/ literals is a good idea. Not quite as ergonomic, but still readable and easy both for the content component producer and consumer. |
Unfortunately, I spoke too soon about using Literals. Don't think it will work. Was hoping something like this could work. But get `(Unknown) -> Any). Passing in a dynamic literal does work when not wrapped in the Callable. But unfortunately not too useful that way. So I'll just go ahead and implement the c.header(), c.footer()
|
I'd avoid namedtuple because it's not great API-wise for long-term maintenance compared to dataclass, see: https://snarky.ca/dont-use-named-tuples-in-new-apis/ Similar to how we do state class, what if we did this? @me.slotclass
class ScaffoldSlots:
header: me.NamedSlot
body: me.NamedSlot
footer: me.NamedSlot
@me.content_component(named_slots=ScaffoldSlots)
def ... Essentially slotclass is an alias for dataclass, but we could do things like enforce [kw_only](https://docs.python.org/3/library/dataclasses.html#:~:text=in%20version%203.10.-,kw_only,-%3A%20If%20true%20(the). Even though this is a bit more verbose than namedtuple, this should be pretty familiar to Mesop developers since it's quite similar to the stateclass syntax and it avoids the downsides of namedtuples (e.g. indexing). |
In terms of the namedtuple thing. I definitely agree about avoiding them. I was mainly using them since they apparently do have some typing support for dynamic creation use cases. Which I thought I could leverage with Literal to keep things less verbose. Yeah, I did consider the dataclass option, but felt it would be too verbose. But I do like your version of it with the slotclass and the type alias for DetachedNodeTreeStateContext. Has a good symmetry and consistency to it. I think that's more of a personal thing, having gotten used to working with python 2.7 and not have type hints most of the time. So not having the type hints doesn't bother me as much as too much boilerplate (I think that's one reason python initially got popular, because the syntax is relatively quick to write in comparison to say Java, but I think that was more a benefit in the past than in the present. The extra verbosity isn't that much of a problem these days with AI assistance to autocomplete boilerplate like this. And I guess, if people were to make their custom components more shareable, it would be more user friendly for users to know what slots existed without having to look at the code (or docs if they exist) Part of me wants to suggest making passing in the slotclass optional but I think that flexibility sort of muddles the API to have multiple ways of doing things. So ok, I'll go with adding slotclass. Thanks for the brainstorming help. |
6ed2b42
to
adb8dd3
Compare
I still need to add tests and maybe some more example test cases to make sure it covers all the edge cases correctly.
But before I do that, just wanted to make sure the API seems OK.
Changes
Issues