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.

Sign up for GitHub

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

Jump to bottom

Rename and update docs for ObjectService ServerData and ClientData #4535

Merged
merged 5 commits into from
Sep 29, 2023
Merged

Rename and update docs for ObjectService ServerData and ClientData #4535

merged 5 commits into from
Sep 29, 2023

Conversation

devinrsmith
Copy link
Member

No description provided.

@devinrsmith devinrsmith self-assigned this Sep 22, 2023
@devinrsmith devinrsmith marked this pull request as ready for review September 22, 2023 16:55
@devinrsmith
Copy link
Member Author

As discussed in #4490 (comment)

@devinrsmith devinrsmith changed the title Rename and update docs for ObjectServer ServerData and ClientData Rename and update docs for ObjectService ServerData and ClientData Sep 22, 2023
@devinrsmith
Copy link
Member Author

I don't think the python client tests are sufficient to ensure this update is valid - it doesn't exercise the messageStream with references for testing. The proposed Echo plugin in #4490 would be a good candidate for the python client to test against.

@niloc132
Copy link
Member

Don't forget to regenerate the gwt bindings - the JS code for the protos will be generated, so methods will be missing. Smoke testing is possible for the python server and js client with any of the existing plotly/matplotlib stuff, and for the python client with the existing runfunc plugins.

@devinrsmith
Copy link
Member Author

I forget how to do the JS code updates...

rcaudy
rcaudy reviewed Sep 22, 2023
View reviewed changes
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

rcaudy
rcaudy reviewed Sep 22, 2023
View reviewed changes
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

rcaudy
rcaudy previously approved these changes Sep 22, 2023
View reviewed changes
jmao-denver
jmao-denver previously approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devinrsmith devinrsmith dismissed stale reviews from jmao-denver and rcaudy via 33d5bbd September 25, 2023 18:10
@devinrsmith
Copy link
Member Author

Tested against https://github.com/deephaven/deephaven.io/pull/3175

@devinrsmith
Copy link
Member Author

Tested against https://github.com/deephaven/deephaven.io/pull/3087

@devinrsmith devinrsmith merged commit e76bf7f into deephaven:main Sep 29, 2023
10 checks passed
@devinrsmith devinrsmith deleted the update-data-to-client-server-data branch September 29, 2023 21:53
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chipkent chipkent chipkent approved these changes

@rcaudy rcaudy rcaudy approved these changes

@jmao-denver jmao-denver jmao-denver left review comments

@nbauernfeind nbauernfeind Awaiting requested review from nbauernfeind nbauernfeind is a code owner

@niloc132 niloc132 Awaiting requested review from niloc132 niloc132 is a code owner

Assignees

@devinrsmith devinrsmith

Labels
NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devinrsmith @niloc132 @jmao-denver @chipkent @rcaudy