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

RSDK-9174: add app wrappers #4561

Open
wants to merge 76 commits into
base: main
Choose a base branch
from

Conversation

purplenicole730
Copy link
Member

@purplenicole730 purplenicole730 commented Nov 18, 2024

Adding all the app wrappers.

Notes:

  • I did add multiple files to try and split up the file a little between the wrappers and types as there are just so many. I can easily consolidate app_proto_conversions.go into app_client.go if preferred.
  • There are also some shadow types that are used in ML Training in registry_proto_conversions.go. I kept this here since ML Training will be using this too, so I'll probably use this file and rename it as the ML Training client. Anything ML Training does not use when we introduce the ML Training wrappers will probably go back into app_proto_conversions.go.
  • Talking to @benjirewis and @jckras, we came to the conclusion that for TailRobotPartLogs, we can give the stream object itself to the user so that the user does what they want with it, whether they want to make it a channel or not. This seems to be common practice in Go.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Looking awesome; this is a large amount of work thanks so much! Two comments: I think we should use a different strategy for "optional" fields, and I have some comments about your streaming code I can relay offline.

app/app_client.go Outdated Show resolved Hide resolved
app/app_types.go Outdated Show resolved Hide resolved
app/registry.go Outdated Show resolved Hide resolved
app/stream.go Outdated Show resolved Hide resolved
app/stream.go Outdated Show resolved Hide resolved
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@purplenicole730 purplenicole730 requested review from benjirewis, a team, jckras, lia-viam and stuqdog and removed request for a team November 21, 2024 21:58
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants