feat: Add generic Stream interface and improve code quality #6
Conversation
hanzeINGH
commented
Dec 27, 2024
•
hanzeINGH
commented
- Implement generic Stream interface for streaming APIs
- Refactor 'chats' to 'chat' for consistency
- Integrate gofumpt check into CI pipeline
WalkthroughThis pull request introduces several significant changes across the Coze Go SDK, primarily focusing on refactoring method and struct names from plural to singular forms (e.g., Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Change-Id: I6dd7a13e0f7dc63353ebc16ca164250ea32c2967
chyroc
changed the title
Change-Id: Id288bf098cc8999256e806ad474610b04b8a13a3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (22)
base_model.go (1)
5-7: Add brief doc comments for clarity.The new
Responserinterface is straightforward, but it would be helpful to document its purpose. Consider adding a short comment to explain when and how to implementResponse() HTTPResponse.stream_reader.go (1)
15-19: Great abstraction for a generic stream.Introducing
Stream[T]simplifies handling of chat or workflow events. However, consider adding doc comments summarizing howResponserandClose()/Recv()interplay to help future maintainers.client.go (2)
10-10: Field rename aligns with the new singular naming convention.
ChangingChatstoChatis coherent with the rest of the refactor.Consider renaming the type from
chattoChatfor complete consistency if that doesn’t conflict with other naming patterns.
80-80: Constructor name mismatch
While the field is now singular, the constructor function remainsnewChats. For consistency:- Chat: newChats(core), + Chat: newChat(core),examples/chats/chat/main.go (2)
70-70: Ensure thorough error reporting for cancellation failures.
Consider adding more detailed logs or user-facing messages in casecozeCli.Chat.Cancelfails. This will help in diagnosing partial cancellations or other errors in production environments.
110-110: Use context-based timeouts in addition to poll timeouts.
You could further enhance reliability by deriving a context with a timeout (viacontext.WithTimeout) forCreateAndPoll, ensuring you can gracefully handle cases when operations exceed the poll duration.examples/auth/pkce_oauth/main.go (1)
70-71: Remove or justify commented-out code.
Leaving commented-out lines (such as printingoauthURL.AuthorizationURL) can clutter the file. If no longer required, consider removing them to keep the code clean.chats.go (11)
Line range hint
13-25: Ensure consistent naming in request and response structs.
Currently, the method name isCreatebut the request/response structs retain plural naming (CreateChatsReq,CreateChatsResp). Consider renaming them to align with the new singular convention (CreateChatReq,CreateChatResp) for clarity.
Line range hint
27-47: Timeout logic might benefit from configurable polling interval.
The polling interval is hardcoded totime.Sleep(time.Second). If the chat needs more frequent or less frequent checks, a configurable interval (e.g.,pollInterval) would make it more flexible without rewriting this function.
47-47: Consider adding context to the cancellation step.
When cancellation fails, the log message is helpful, but you might also want to return a more descriptive error upstream with relevant context, such as the conversation and chat IDs.
97-104: Constructor naming mismatch.
type chatand the constructornewChatsis slightly inconsistent. Consider renaming the constructor tonewChator the struct name tochatsfor consistency.
Line range hint
161-176: SubmitToolOutputs: Only partial error logging.
If the function returns an error, there is no attempt to log any partial context about the request. Logging might help debugging in production environments.
Line range hint
177-188: Duplicated streaming pattern.
StreamSubmitToolOutputsreuses almost the same streaming logic asStream. This duplication is fine if it’s minimal, but consider factoring out a shared helper if more streaming functionalities are planned.
212-215: Minor mismatch in naming:
ChatStatusCancelledis assigned the string"canceled"rather than"cancelled". This is purely stylistic but be consistent across the codebase.
Line range hint
330-357: Check for alignment with rename from plural to singular.
CreateChatsReqreferences “Chats.” If usage is indeed multiple, it’s fine. If it references a single chat, consistent naming might help.
358-365: Ensure doc lines match code changes.
The doc blocks mention “can be viewed in the 'conversation_id' field of the Response” or “chat is.” Confirm these references are accurate after rename from plural to singular.
Line range hint
386-395: Doc references forSubmitToolOutputsChatReq.
Same note about rename fromchatstochat.
Line range hint
396-406: Nested responses appear consistent.
createChatsRespwrapsChat *CreateChatsResp. Just note the naming mismatch in “createChatsResp” vs. “CreateChatsResp.”.github/workflows/ci.yml (1)
37-48: Eliminate trailing spaces detected by lint.
Trailing spaces at line 43 can cause the lint to fail. Remove them to keep the config tidy.- gofumpt -l -w . + gofumpt -l -w .🧰 Tools
🪛 yamllint (1.35.1)
[error] 43-43: trailing spaces
(trailing-spaces)
README.md (3)
Line range hint
103-114: Update request struct name in the example.
The code snippet referencescoze.CreateChatReqwhile the actual struct in the code is still namedCreateChatsReq. Update the example for consistent naming.
Line range hint
135-156: Display partial completion info.
Printing messages as they come in is correct. You might consider indicating partial completion with a newline or spinner for clarity.
228-228: Example references still mentionChats.
Change “Chats” to “Chat” if you align with the new singular naming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/ci.yml(1 hunks)CONTRIBUTING.md(1 hunks)README.md(4 hunks)audio_rooms_test.go(0 hunks)audio_voices_test.go(0 hunks)base_model.go(1 hunks)bots_test.go(0 hunks)chats.go(22 hunks)chats_messages.go(1 hunks)chats_messages_test.go(0 hunks)chats_test.go(2 hunks)client.go(2 hunks)client_test.go(1 hunks)conversations_messages_test.go(0 hunks)conversations_test.go(0 hunks)datasets_documents_test.go(0 hunks)examples/auth/pkce_oauth/main.go(2 hunks)examples/chats/chat/main.go(5 hunks)examples/chats/chat_with_image/main.go(2 hunks)examples/chats/stream/main.go(2 hunks)examples/chats/submit_tool_output/main.go(3 hunks)examples/client/log/main.go(1 hunks)examples/workflows/runs/stream/main.go(2 hunks)files_test.go(0 hunks)stream_reader.go(2 hunks)workflows_runs.go(1 hunks)workflows_runs_histories_test.go(0 hunks)workflows_runs_test.go(0 hunks)workspaces_test.go(0 hunks)
💤 Files with no reviewable changes (11)
- workflows_runs_test.go
- workspaces_test.go
- audio_voices_test.go
- workflows_runs_histories_test.go
- files_test.go
- conversations_messages_test.go
- datasets_documents_test.go
- chats_messages_test.go
- bots_test.go
- conversations_test.go
- audio_rooms_test.go
✅ Files skipped from review due to trivial changes (2)
- chats_messages.go
- CONTRIBUTING.md
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci.yml
[error] 43-43: trailing spaces
(trailing-spaces)
🔇 Additional comments (32)
stream_reader.go (1)
84-84: Method rename aligns well with the Responser contract.
Renaming HTTPResponse() to Response() keeps naming consistent with the newly introduced interface. Good job!
examples/chats/chat_with_image/main.go (2)
58-58: Looks consistent with the new singular API naming convention.
Transitioning from Chats.Stream to Chat.Stream appears correct and aligns with the refactoring goal.
84-84: Response method usage is correctly updated.
Switching from HTTPResponse() to Response() is consistent and appears to be working properly.
examples/workflows/runs/stream/main.go (2)
47-47: Signature update to use the new generic stream type.
Switching from a dedicated WorkflowEventReader to the generic coze.Stream[coze.WorkflowEvent] simplifies the interface.
83-83: Correct usage of the renamed response method.
Invoking resp.Response().LogID() is aligned with the new naming convention.
examples/chats/submit_tool_output/main.go (3)
38-38: Method name updated to singular.
Replacing cozeCli.Chats.Stream with cozeCli.Chat.Stream matches the rest of the refactoring.
94-94: Tool output streaming method rename is correct.
Updating to Chat.StreamSubmitToolOutputs is consistent with the singular naming.
118-118: Response method usage confirmed.
Leveraging resp.Response().LogID() aligns with the new singular approach for response handling.
examples/chats/chat/main.go (3)
44-44: Consistent adoption of the singular Chat interface.
The renamed function call cozeCli.Chat.Create is aligned with the singular naming convention and helps maintain consistency across the refactored API.
82-82: Validate missing conversation or chat edge cases.
Before retrieving a chat, you may consider additional validation or error handling for non-existent conversation_id or chat_id, especially in production deployments.
101-101: Singular interface usage in CreateAndPoll is correct.
The usage of cozeCli.Chat.CreateAndPoll is confirmed to be the expected interface. Implementation looks correct.
client_test.go (1)
31-31: Correct field reference to api.Chat.
This test now properly checks the renamed Chat field, aligning with the new singular naming convention in the codebase. Looks good.
examples/auth/pkce_oauth/main.go (1)
82-85: Improved parameter formatting for clarity.
Neatly aligning parameters in oauth.GetAccessToken improves readability and follows consistent code style practices. Implementation looks fine.
chats_test.go (2)
175-175: Updated event name matches singular chat refactor.
Renaming the event to conversation.chat.created keeps it consistent with the singular Chat domain model. This enhances clarity in the streaming protocols.
343-343: Use of conversation.chat.in_progress event is consistent.
Changing to a singular event name mirrors the new interface and keeps the streaming responses uniform.
chats.go (14)
Line range hint 79-93: Streaming logic looks correct.
The code sets up a buffered reader and a parser function without obvious data races, concurrency issues, or missing error checks.
Line range hint 134-145: Potential concurrency concerns if used by multiple goroutines.
If multiple goroutines call Cancel concurrently, ensure that the underlying HTTP client usage is thread-safe (most default HTTP clients in Go are). This function itself appears safe, but consider clarifying concurrency assumptions in code comments if relevant.
Line range hint 146-160: Retrieve function’s partial usage of query parameters.
You are using withHTTPQuery("conversation_id", req.ConversationID). Ensure that calls that rely on other optional parameters also pass them in, if required (or consider a more general approach to constructing query parameters).
189-194: Validated streaming approach.
The streamReader[...] usage is consistent with Stream above. No immediate issues found.
Line range hint 216-241: Event naming is consistent and descriptive.
The enumerated event names properly match typical conversation lifecycle states.
Line range hint 242-278: Struct fields appear logically grouped.
No major issues found. The usage of MetaData, Status, and RequiredAction is intuitive and well-documented.
Line range hint 279-292: Token usage fields are sufficiently descriptive.
They clearly separate total tokens, output tokens, and input tokens.
Line range hint 293-329: Use caution with partially filled ChatSubmitToolOutputs.
Ensure that fields like ToolCalls are validated or defaulted if not provided.
369-376: Doc references for Retrieve are consistent.
No code issues found. The doc references match the new singular usage.
Line range hint 407-417: cancelChatsResp similarly structured.
No immediate issues found. Pay attention to consistent naming across multiple declarations.
Line range hint 418-439: retrieveChatsResp similarly structured.
No immediate issues found.
440-442: Comment references correct struct.
ChatEvent includes Chat *Chat. Looks correct.
Line range hint 443-475: Parsing logic for events is robust.
doParseChatEvent checks event strings and uses JSON to parse data. Seems reliable for typical events.
476-478: ChatPoll struct is a clean approach for poll results.
Storing both Chat and Messages together is clear for consumers.
README.md (1)
Line range hint 117-138: Unify streaming example with code.
The example references CreateChatReq for streaming. Ensure actual usage matches the code, which references CreateChatsReq.
workflows_runs.go (2)
41-54: Stream function updated similarly.
Consistent with other Stream changes. Verify that consumers are prepared for the new interface.
24-37: Method signature updated for Resume.
Refactoring from (*WorkflowEventReader, error) to (Stream[WorkflowEvent], error) is consistent with the new streaming approach. Make sure all call sites are updated.
✅ Verification successful
Let me verify the call sites by checking the actual usage of the Resume method.
All call sites are properly updated to use the new streaming interface
After reviewing both the test file and example implementation, I can confirm that all call sites are correctly using the new streaming interface. Both locations where Resume is called:
- In
workflows_runs_test.go: The test correctly uses the new streaming interface, properly callingRecv()to read events from the stream. - In
examples/workflows/runs/stream/main.go: The example code correctly handles the returnedStream[WorkflowEvent]interface, passing it tohandleEventswhich properly uses the streaming methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking references to the old function signature
rg -A 3 'ResumeRunWorkflowsReq'
Length of output: 1228
Script:
#!/bin/bash
# Check the implementation of test and example files that use Resume
cat workflows_runs_test.go
cat examples/workflows/runs/stream/main.go
Length of output: 9876
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
27-28: Combine dependency installation steps and fix formattingThe installation steps should be combined for better efficiency, and trailing spaces should be removed.
Apply this diff:
- name: Install dependencies - run: go mod download - - go install mvdan.cc/gofumpt@v0.5.0 + run: | + go mod download + go install mvdan.cc/gofumpt@v0.5.0🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)examples/auth/pkce_oauth/main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/auth/pkce_oauth/main.go
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci.yml
[error] 27-27: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
40-45: LGTM! Well-implemented formatting check
The gofumpt check implementation is robust:
- Uses appropriate flags for diff and error reporting
- Provides clear error messaging
- Ensures output visibility through tee
- Positioned appropriately in the workflow sequence
Line range hint 13-13: Verify Go version compatibility
Please verify that Go 1.23 is officially released and supported. If not, consider removing it from the version matrix until it's officially released.
🧰 Tools
🪛 actionlint (1.7.4)
31-31: the runner of "golangci/golangci-lint-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 27-27: trailing spaces
(trailing-spaces)
[error] 29-29: trailing spaces
(trailing-spaces)
