fix(sdk/go): list Go SDK in READMEs and fix two review findings#1767
Conversation
|
@MarcusElwin is attempting to deploy a commit to the motia Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds inbound UnregisterTrigger handling to the Go SDK client, ensures integration tests target a specific worker by name, and updates root and SDK README files to include and document the Go SDK. ChangesGo SDK Trigger Unregistration and Integration Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/README.md`:
- Around line 131-133: The table/text incorrectly claims all SDKs auto-connect
on initialization; update the README to clarify Go requires an explicit Connect
call by changing the "Initialize" cell for Go from "iii.RegisterWorker(url)" to
indicate registration only (e.g., "iii.RegisterWorker(url) — register only; call
Connect(...) to connect") and adjust the summary lines (around the paragraphs
referencing auto-connect) to note that Go does not auto-connect and requires an
explicit iii.Connect or Connect call after iii.RegisterWorker; ensure references
to iii.RegisterWorker and Connect are used so readers can find the Go example
easily.
- Around line 90-127: Remove the inline Go example block in sdk/README.md (the
code that uses RegisterWorker, RegisterFunction, RegisterTrigger and Connect)
and move that exact example into a dedicated docs example file; then replace the
README code block with a short link/reference to the new docs example so the
README points to the canonical example in docs rather than embedding the full
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df1859c0-514d-4b82-848f-a3ccf6bc765f
📒 Files selected for processing (6)
README.mdsdk/README.mdsdk/packages/go/iii/client.gosdk/packages/go/iii/client_test.gosdk/packages/go/iii/tests/common_test.gosdk/packages/go/iii/tests/worker_metadata_test.go
| ### Go | ||
|
|
||
| ```go | ||
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "log" | ||
|
|
||
| iii "github.com/iii-hq/iii/sdk/packages/go/iii" | ||
| ) | ||
|
|
||
| func main() { | ||
| client := iii.RegisterWorker("ws://127.0.0.1:49134") | ||
|
|
||
| client.RegisterFunction("hello::greet", func(ctx context.Context, data json.RawMessage) (any, error) { | ||
| var req struct { | ||
| Body struct { | ||
| Name string `json:"name"` | ||
| } `json:"body"` | ||
| } | ||
| _ = json.Unmarshal(data, &req) | ||
| return map[string]any{ | ||
| "status_code": 200, | ||
| "body": map[string]string{"message": "Hello, " + req.Body.Name + "!"}, | ||
| }, nil | ||
| }) | ||
|
|
||
| client.RegisterTrigger("hello-http", "http", "hello::greet", | ||
| json.RawMessage(`{"api_path":"/greet","http_method":"POST"}`), nil) | ||
|
|
||
| if err := client.Connect(context.Background()); err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| defer client.Close() | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move README inline example to docs and link it instead.
This section embeds a full Go example in sdk/README.md. The repository guideline for **/README.md says examples should live in docs/ and READMEs should link to them to avoid drift.
As per coding guidelines, “READMEs should not contain example code that is already in the docs/. READMEs should link these examples in the docs/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/README.md` around lines 90 - 127, Remove the inline Go example block in
sdk/README.md (the code that uses RegisterWorker, RegisterFunction,
RegisterTrigger and Connect) and move that exact example into a dedicated docs
example file; then replace the README code block with a short link/reference to
the new docs example so the README points to the canonical example in docs
rather than embedding the full code.
| | Operation | Node.js | Python | Rust | Go | Description | | ||
| | ------------------------ | ---------------------------------------------------- | ------------------------------------------- | -------------------------------------------- | ------------------------------------------- | ------------------------------------------------------ | | ||
| | Initialize | `registerWorker(url)` | `register_worker(url, options?)` | `register_worker(url, options)` | `iii.RegisterWorker(url)` | Create an SDK instance and auto-connect | |
There was a problem hiding this comment.
Fix Go initialization semantics in the API table/text.
Line 133 and Line 139 state initialization auto-connects across all SDKs, but this file’s Go example (Line 122) requires explicit Connect(...). Please adjust the table/summary to avoid misleading Go users.
Proposed doc fix
-| Initialize | `registerWorker(url)` | `register_worker(url, options?)` | `register_worker(url, options)` | `iii.RegisterWorker(url)` | Create an SDK instance and auto-connect |
+| Initialize | `registerWorker(url)` | `register_worker(url, options?)` | `register_worker(url, options)` | `iii.RegisterWorker(url)` | Create an SDK instance (Go connects via `Connect(ctx)`) |-`registerWorker()` / `register_worker()` creates an SDK instance and auto-connects to the engine. It handles WebSocket communication, automatic reconnection, and OpenTelemetry instrumentation. All four SDKs expose the same API surface — register functions and triggers, then invoke them.
+`registerWorker()` / `register_worker()` creates an SDK instance. Node/Python auto-connect; Go connects explicitly via `Connect(ctx)`. SDKs expose the same core API surface — register functions and triggers, then invoke them.Also applies to: 139-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/README.md` around lines 131 - 133, The table/text incorrectly claims all
SDKs auto-connect on initialization; update the README to clarify Go requires an
explicit Connect call by changing the "Initialize" cell for Go from
"iii.RegisterWorker(url)" to indicate registration only (e.g.,
"iii.RegisterWorker(url) — register only; call Connect(...) to connect") and
adjust the summary lines (around the paragraphs referencing auto-connect) to
note that Go does not auto-connect and requires an explicit iii.Connect or
Connect call after iii.RegisterWorker; ensure references to iii.RegisterWorker
and Connect are used so readers can find the Go example easily.
|
This looks good, @MarcusElwin. We really appreciate your attention to details. I just added a couple of very small comments |
Co-authored-by: Sérgio Marcelino <sergio@filho.org>
Co-authored-by: Sérgio Marcelino <sergio@filho.org>
Thanks @sergiofilhowz committed your suggestions. |
|
Thanks again @MarcusElwin!! We're good to merge this one too! |
2 participants
Closes #1765
Closes #1766
What
Three changes to the (now-merged) Go SDK:
client.go) fix (which was ignored before)tests/worker_metadata_test.go)Why
The Go SDK shipped in #1739 but was missing from the READMEs, so it was undiscoverable
next to the other SDKs. The other two are correctness fixes from the #1740 review
(CodeRabbit): the teardown gap is a real resource leak for custom trigger types, and the
test could give a false green.
Notes
TestInboundUnregisterTrigger(unit, race-clean) covers the teardowndispatch;
TestWorkerRegistersWithGoRuntimepasses against a live engine with thename-correlated assertion. Both README Go snippets compile against the merged SDK.
trigger_type, callUnregisterTriggerwith the instance id (the wire messagecarries only
id+ optionaltrigger_type, so the handler keys cleanup off the id).Summary by CodeRabbit
New Features
Documentation
Tests