-
Notifications
You must be signed in to change notification settings - Fork 3
Implements Spawn
in Go and TS
#12
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
base: main
Are you sure you want to change the base?
Conversation
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.
Specific review comments are below, but there are a couple general style changes that I think would make it a better library:
- Follow doc comment conventions: complete sentences, and start with the name of the item being described. https://tip.golang.org/doc/comment
- Fix typos and incomplete comments that trailed off
- Use
%w
for Go errors instead of%v
These should be pretty quick changes though. After that, happy to collaborate on porting to JS and then we can merge both together.
@ekzhang thanks for the review. Let me know what you think now. |
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.
Nice!
Spawn
in the Go SDKSpawn
in Go and TS
Co-authored-by: Eric Zhang <[email protected]>
Co-authored-by: Eric Zhang <[email protected]>
@ekzhang can you take another look? My latest changes implement timeout following our conventions and using the implementation from the Python SDK as reference. |
The Go SDK now supports spawn as follows:
Internally,
Remote()
is effectively the same asSpawn()
+Get()
.Equivalent TS implementation: