Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Sep 19, 2024

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

import dataclasses

@dataclasses.dataclass
class Databar:
    column: str
    color: list[str] | None = None

from deephaven import time_table, ui

_t = time_table("PT1S").update(["X=ii", "Y=X", "Z=X", "A=X", "B=X", "C=X", "HalfX=X/2", "LogX=X > 0 ? log(X) : 0", "X2=X*2"])

t = ui.table(_t, databars=[
    Databar(column="B"),
])

Copy link
Member

@mofojed mofojed left a 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}"

https://github.com/deephaven/deephaven-plugins/pull/810/files#diff-5e86d7b937ea83974fe4548642726e5c876a1e47aaa9ea54fb246cd43ae192a8

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))
Copy link
Member

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.

Comment on lines 75 to 77
else:
logger.debug("render_item returning child (%s): %s", type(item), item)
return item
Copy link
Member

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.

@mofojed
Copy link
Member

mofojed commented Sep 20, 2024

Should be able to add a unit test in test_renderer.py, and/or e2e tests as well if you want to add some tests for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dataclasses for Deephaven UI
2 participants