-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Dataclass serialization support for deephaven UI #897
base: main
Are you sure you want to change the base?
Conversation
4588b3d
to
de2b4bb
Compare
de2b4bb
to
3ab5d98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wusteven815 has some changes about to merge as well, just need to make sure his changes in Renderer are captured, in particular his changes to _get_context_key
:
if isinstance(item, Element):
key = item.key
return key if key is not None else f"{index_key}-{item.name}"
return ( | ||
_render_item(item, context.get_child_context(key)) if key is not None else item | ||
) | ||
return _render_item(item, context.get_child_context(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check here before creating child contexts unnecessarily... pretty much if _render_item
is going to fall into the last case where it just returns item
instead of rendering into a context, we shouldn't bother creating the child context.
else: | ||
logger.debug("render_item returning child (%s): %s", type(item), item) | ||
return item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really need to be in an else
, but paired with my other comment you may want to remove this entirely depending on how you clean it up.
Should be able to add a unit test in |
Fixes #757
Not really sure how to best test this in Python unit tests given our test setup. I tested manually by creating my own dataclass with this code