Convenience Changes, Removal of Plain Send() to Avoid Confusion
#349
Conversation
WalkthroughThis pull request introduces significant changes to the WebSocket client implementation in the Deepgram API. The Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
Deepgram/Clients/Listen/v2/WebSocket/Constants.cs (2)
15-18: Add XML documentation for the new message type constants.The new constants look good and align with the PR's goal of making message types more explicit. However, consider adding XML documentation to describe the purpose and usage of each constant.
// Default flush period public const int DefaultFlushPeriodInMs = 500; - // user message types + /// <summary> + /// Constants representing different types of user control messages for the WebSocket connection. + /// </summary> + + /// <summary> + /// Message type for keeping the connection alive. + /// </summary> public const string KeepAlive = "KeepAlive"; + + /// <summary> + /// Message type for finalizing the stream. + /// </summary> public const string Finalize = "Finalize"; + + /// <summary> + /// Message type for closing the stream. + /// </summary> public const string CloseStream = "CloseStream";
Line range hint
7-9: Fix incorrect class-level documentation.The current class documentation refers to "Headers" from the Speak API, but this class is in the Listen namespace and contains WebSocket message type constants.
-/// <summary> -/// Headers of interest in the return values from the Deepgram Speak API. -/// </summary> +/// <summary> +/// Constants used by the Deepgram Listen WebSocket API. +/// </summary>Deepgram/Models/Listen/v2/WebSocket/ControlMessage.cs (1)
7-8: Add XML documentation for the ControlMessage class.Add class-level XML documentation to describe the purpose and usage of this control message class.
+/// <summary> +/// Represents a WebSocket control message used for managing the WebSocket connection state. +/// </summary> public class ControlMessage(string text)Deepgram/Utilities/QueryParameterUtil.cs (1)
49-49: LGTM! Consider adding XML documentation for nullable parameter.The change to make the parameters nullable is a good improvement that makes the API more explicit about null handling. However, since this is an internal utility method that handles null parameters differently, it would be helpful to document this behavior.
Add XML documentation to clarify the nullable parameter behavior:
/// <summary> /// Encodes the specified parameters into a URL /// </summary> + /// <param name="parameters">The parameters to encode. If null, only addons will be encoded.</param> + /// <param name="propertyInfoList">The properties to encode. If null, no properties will be encoded.</param> + /// <param name="addons">Additional key-value pairs to encode. If null, no addons will be encoded.</param> + /// <returns>The URL-encoded string representation of the parameters.</returns> internal static string UrlEncode<T>(T? parameters, IEnumerable<PropertyInfo>? propertyInfoList, Dictionary<string, string>? addons = null)Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (4)
15-17: Enhance XML documentation for Connect method.The documentation should be more comprehensive. Consider adding:
- Parameter descriptions for
options,cancelToken,addons, andheaders- Return value documentation explaining what the boolean indicates
/// <summary> -/// Connects to the Deepgram WebSocket API +/// Connects to the Deepgram WebSocket API for real-time audio transcription. /// </summary> +/// <param name="options">The configuration options for the WebSocket connection.</param> +/// <param name="cancelToken">Optional cancellation token to cancel the connection attempt.</param> +/// <param name="addons">Optional dictionary of addon parameters.</param> +/// <param name="headers">Optional dictionary of custom headers to include in the connection request.</param> +/// <returns>True if the connection was established successfully, false otherwise.</returns>
21-23: Enhance XML documentation for Stop method.The documentation should explain the parameters and return value.
/// <summary> -/// Disconnects from the Deepgram WebSocket API +/// Disconnects from the Deepgram WebSocket API and cleans up resources. /// </summary> +/// <param name="cancelToken">Optional cancellation token to cancel the disconnection attempt.</param> +/// <param name="nullByte">When true, sends a null byte before closing the connection.</param> +/// <returns>True if disconnected successfully, false otherwise.</returns>
Line range hint
89-92: Remove redundant Send method.As per PR objectives, the plain
Send()method should be removed to avoid confusion between binary and text messages. TheSendBinary()method should be used instead.Remove this method as it duplicates
SendBinary()functionality:- /// <summary> - /// Sends a binary message over the WebSocket connection. - /// </summary> - /// <param name="data">The data to be sent over the WebSocket.</param> - public void Send(byte[] data, int length = Constants.UseArrayLengthForSend);
Line range hint
110-120: Follow C# naming conventions for parameters.The parameter
_cancellationTokenuses an underscore prefix, which doesn't follow C# naming conventions. Parameters should use camelCase without underscores.Update the parameter names in both methods:
- public Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null); + public Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null); - public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null); + public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null);Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (2)
Remove
Sendmethod and uncommentSendBinaryin ISpeakWebSocketClientThe verification reveals:
- The generic
Sendmethod in v1/v2 interfaces is currently forwarding to eitherSendMessageorSendBinaryin implementationsIListenWebSocketClientalready has bothSendBinaryandSendMessagemethods activeISpeakWebSocketClienthasSendBinarycommented out, but it should be available like inIListenWebSocketClientRequired changes:
- Remove the generic
Sendmethod from both v1/v2ISpeakWebSocketClientinterfaces- Uncomment the
SendBinarymethod in both v1/v2ISpeakWebSocketClientinterfaces- Update implementations to directly use
SendBinary/SendMessageinstead of the genericSend🔗 Analysis chain
Line range hint
120-124: Remove the generic Send method as per PR objectives.The PR's main objective is to remove the plain
Send()method to avoid confusion between binary and text WebSocket messages. However, this method is still present in the interface. Consider:
- Removing this method
- Ensuring all implementations use either
SendMessagefor text or uncomment and implementSendBinaryfor binary messagesLet's verify the usage of this method across the codebase:
Let me gather more information about the implementation details to understand the full picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the Send method to ensure safe removal rg -A 2 "Send\(byte\[\] data, int length = Constants\.UseArrayLengthForSend\)"Length of output: 1718
Script:
#!/bin/bash # Check for SendMessage and SendBinary implementations rg -A 2 "SendMessage\(byte\[\]|SendBinary\(byte\[\]" # Check for any commented implementations rg -B 2 "//.*Send(Message|Binary)\("Length of output: 5404
Remove commented-out methods as they are already implemented in the abstract class
The commented-out methods
SendBinaryandSendBinaryImmediatelyin the interface are already properly implemented inAbstractWebSocketClient.csand available to all client implementations. The v2 Listen client uses these methods, while the v1 Speak client has its own commented-out versions that should be removed to avoid confusion.
- Remove commented-out methods in
ISpeakWebSocketClient.csas they are redundant with the abstract implementation- Clean up commented-out implementations in
Deepgram/Clients/Speak/v1/WebSocket/Client.csto avoid maintaining dead code🔗 Analysis chain
Line range hint
95-97: Clarify the status of commented-out methods.Several methods are commented out, including
SendBinaryandSendBinaryImmediately, which seem relevant to the PR's objective of distinguishing between binary and text messages. Please clarify:
- Are these methods intended to be implemented as part of this PR?
- If not, should they be removed entirely rather than left commented out?
Let's check if these methods are implemented in any concrete classes:
Also applies to: 108-111, 132-136
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for implementations of these methods ast-grep --pattern 'public void SendBinary(byte[] $_, int $_) { $$$ }' ast-grep --pattern 'public Task SendBinaryImmediately(byte[] $_, int $_) { $$$ }'Length of output: 156
Script:
#!/bin/bash # Let's try a broader search for these method names and their implementations rg -l "SendBinary|SendBinaryImmediately" --type cs # Also search for any commented implementations rg -U "(?s)/\*.*?(SendBinary|SendBinaryImmediately).*?\*/" --type cs # Look for interface definitions rg "interface.*?ISpeakWebSocketClient" --type cs -A 5Length of output: 1695
Script:
#!/bin/bash # Let's examine the actual implementations in the v2 clients rg -A 10 "SendBinary|SendBinaryImmediately" Deepgram/Clients/Speak/v1/WebSocket/Client.cs Deepgram/Clients/Listen/v2/WebSocket/Client.cs # Also check the abstract class implementation rg -A 10 "SendBinary|SendBinaryImmediately" Deepgram/Abstractions/v2/AbstractWebSocketClient.csLength of output: 6581
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (1)
297-301: Consider enhancing the Send method documentation and validation.While the implementation correctly handles binary messages, there are a few suggestions for improvement:
- Add XML documentation for the
lengthparameter.- Consider adding null/empty array validation.
- Consider renaming the method to
SendBinaryDatato align with the PR's objective of avoiding confusion between binary and text messages.Here's the suggested implementation:
/// <summary> /// Sends a binary message over the WebSocket connection. /// </summary> /// <param name="data">The data to be sent over the WebSocket.</param> + /// <param name="length">The number of bytes to send. If not specified, the entire array length is used.</param> + /// <exception cref="ArgumentNullException">Thrown when data is null.</exception> + /// <exception cref="ArgumentException">Thrown when data array is empty.</exception> - public void Send(byte[] data, int length = Abstract.Constants.UseArrayLengthForSend) => SendBinary(data, length); + public void SendBinaryData(byte[] data, int length = Abstract.Constants.UseArrayLengthForSend) + { + ArgumentNullException.ThrowIfNull(data); + if (data.Length == 0) + { + throw new ArgumentException("Data array cannot be empty", nameof(data)); + } + SendBinary(data, length); + }Deepgram/Clients/Speak/v2/WebSocket/Client.cs (2)
391-394: Consider adding XML documentation for the Send method.While the implementation is correct, adding XML documentation would help maintain consistency with other public methods and clarify that this method is specifically for text messages.
+ /// <summary> + /// Sends a text message over the WebSocket connection. + /// This implementation ensures all messages are handled as text messages for the Speak API. + /// </summary> + /// <param name="data">The byte array containing the text message data</param> + /// <param name="length">The length of data to send, defaults to using the full array length</param> public void Send(byte[] data, int length = Abstract.Constants.UseArrayLengthForSend) => SendMessage(data, length);
Line range hint
12-24: Consider implementing ISendWebSocketClient interface.The class implements
ISpeakWebSocketClientbut also provides message sending capabilities. Consider extracting the message sending methods into a separate interface (e.g.,ISendWebSocketClient) to better follow the Interface Segregation Principle.Also applies to: 391-394
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Deepgram/Abstractions/v2/AbstractWebSocketClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs(1 hunks)Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs(1 hunks)Deepgram/Clients/Listen/v2/WebSocket/Client.cs(4 hunks)Deepgram/Clients/Listen/v2/WebSocket/Constants.cs(1 hunks)Deepgram/Clients/Speak/v2/WebSocket/Client.cs(2 hunks)Deepgram/Models/Listen/v2/WebSocket/ControlMessage.cs(1 hunks)Deepgram/Utilities/QueryParameterUtil.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Deepgram/Abstractions/v2/AbstractWebSocketClient.cs
🧰 Additional context used
📓 Learnings (3)
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (1)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:25-70
Timestamp: 2024-10-28T18:14:28.097Z
Learning: In the file `Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs`, for the `Subscribe` methods, when the parameter `eventHandler` is self-explanatory, it's acceptable to omit descriptions in the XML documentation. Therefore, I'll avoid flagging missing parameter descriptions for `eventHandler` in `Subscribe` methods.
Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (2)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs:49-49
Timestamp: 2024-10-28T19:43:32.373Z
Learning: In the 'deepgram-dotnet-sdk' project, existing documentation bugs will be addressed in a later pass, so minor documentation issues in code comments may be deferred during code reviews.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs:61-61
Timestamp: 2024-10-28T19:43:28.767Z
Learning: In the codebase, minor grammatical issues in XML comments, such as articles before words in summary descriptions, can be ignored during code reviews.
Deepgram/Models/Listen/v2/WebSocket/ControlMessage.cs (3)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#316
File: Deepgram/Models/Speak/v1/WebSocket/ControlMessage.cs:7-8
Timestamp: 2024-10-09T02:19:46.086Z
Learning: Primary constructor syntax in class declarations, such as `public class ClassName(Type parameter)`, is valid starting from C# 12.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#316
File: Deepgram/Models/Speak/v1/WebSocket/ControlMessage.cs:7-8
Timestamp: 2024-10-09T02:19:48.728Z
Learning: Primary constructor syntax in class declarations, such as `public class ClassName(Type parameter)`, is valid starting from C# 12.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#316
File: Deepgram/Models/Speak/v1/WebSocket/ControlMessage.cs:7-8
Timestamp: 2024-09-18T17:03:37.231Z
Learning: Primary constructor syntax in class declarations, such as `public class ClassName(Type parameter)`, is valid starting from C# 12.
🔇 Additional comments (9)
Deepgram/Models/Listen/v2/WebSocket/ControlMessage.cs (2)
1-23: Implementation aligns well with PR objectives.
This new ControlMessage class provides a clear structure for control messages, supporting the PR's goal of making message types more explicit and reducing confusion in the WebSocket communication.
19-22:
Review ToString implementation and add missing using statements.
- Add required using statements at the top of the file:
+using System.Text.Json;
+using System.Text.RegularExpressions;- The
Regex.Unescapemight be unnecessary asJsonSerializeralready handles proper JSON string escaping. Consider simplifying:
-return Regex.Unescape(JsonSerializer.Serialize(this, JsonSerializeOptions.DefaultOptions));
+return JsonSerializer.Serialize(this, JsonSerializeOptions.DefaultOptions);Please verify if there's a specific reason for using Regex.Unescape.
Deepgram/Utilities/QueryParameterUtil.cs (1)
49-49: Verify WebSocket callback URL handling.
Since this utility is used for encoding callback URLs and the PR involves WebSocket message handling changes, let's verify that callback URL encoding is properly tested.
✅ Verification successful
No impact on WebSocket callback handling from this change
The nullable parameter change in QueryParameterUtil.UrlEncode is safe because:
- The utility is well-tested with callback URLs as shown in
QueryParameterUtilTests.cs - The method already handles null values correctly for all parameter types (PreRecorded, Analyze, and Speak schemas)
- WebSocket clients (Listen v1/v2, Speak v1/v2) use this utility only for initial URI construction, not for message handling
- The Live client (which is marked as deprecated) inherits from Listen WebSocket client and doesn't interact with URL encoding directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tests covering WebSocket callback URL scenarios
# and verify the callback parameter handling in WebSocket-related code.
echo "Checking for WebSocket callback tests..."
rg -l "WebSocket.*Callback|Callback.*WebSocket" --type-add 'test:*.{cs,vb}' -t test
echo "Checking WebSocket-related code for callback URL usage..."
rg "UrlEncode.*Callback" --type cs
Length of output: 373
Script:
#!/bin/bash
# Let's check the actual usage of QueryParameterUtil.UrlEncode in the codebase
echo "Checking all usages of UrlEncode..."
rg "UrlEncode" -B 2 -A 2 --type cs
# Also check if there are any tests for QueryParameterUtil
echo -e "\nChecking for QueryParameterUtil tests..."
fd "QueryParameterUtil.*Test" --type f
# Let's examine the Live Client implementation since it was found in previous search
echo -e "\nExamining Live Client implementation..."
cat "Deepgram/Clients/Live/v1/Client.cs"
Length of output: 21729
Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (2)
15-17: LGTM! Documentation added for Connect method.
The documentation provides a clear, concise description of the method's purpose.
21-23: LGTM! Documentation added for Stop method.
The documentation provides a clear, concise description of the method's purpose.
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (2)
6-6: LGTM! Clear and purposeful alias import.
The alias improves code readability when referencing constants from the abstractions namespace.
281-282: LGTM! Consistent control message handling.
The use of ControlMessage class standardizes the construction of control messages, improving type safety and maintainability.
Let's verify that the control message constants are properly defined:
Also applies to: 292-293, 334-335
✅ Verification successful
Control message constants are properly defined and used consistently
The verification confirms that all required control message constants (KeepAlive, Finalize, and CloseStream) are properly defined as string constants in the Constants.cs file, ensuring consistent usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify control message constants are defined and consistent
# Expected: Constants for KeepAlive, Finalize, and CloseStream should be defined
rg -A 1 'public const string (KeepAlive|Finalize|CloseStream)'
Length of output: 415
Deepgram/Clients/Speak/v2/WebSocket/Client.cs (2)
393-393: LGTM! The simplified Send method implementation aligns with PR objectives.
The change to directly forward calls to SendMessage helps clarify that this client only deals with text messages, reducing ambiguity between binary and text WebSocket messages.
391-394: Verify the impact of Send method changes.
Let's verify that all callers of the Send method are prepared for text-only message handling.
✅ Verification successful
Let me analyze the results and generate a final response.
The Send method changes are safe and correctly implemented
The verification shows that:
- The Send method in Speak/v2/WebSocket/Client.cs is used correctly for text-only messages
- All binary message handling is properly segregated in different client implementations:
- Listen/v1 and Listen/v2 clients handle binary messages for STT
- Speak/v1 and Speak/v2 clients have separate implementations for TTS
The comment's concern about text-only message handling is addressed by the clear separation of responsibilities between different client implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the Send method in the codebase
# to verify they are compatible with text-only message handling
echo "Searching for direct Send method calls..."
rg -l "\.Send\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "\.Send\(" "$file"
done
echo "Searching for potential binary message handling..."
rg -l "WebSocketMessageType\.Binary" | while read -r file; do
echo "=== $file ==="
rg -A 2 "WebSocketMessageType\.Binary" "$file"
done
Length of output: 4793
Proposed changes
This change includes:
Send()to Avoid Confusion as it does not describe the message is binary or text WS message. This is important because in Agent API you will be sending a lot of both Binary and Text messages andSend()becomes too vagueTypes of changes
What types of changes does your code introduce to the community .NET SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit
New Features
SendBinaryandSendMessagemethods for improved message handling in WebSocket communication.ControlMessageclass to enhance control message management.Clientclass.Documentation
IListenWebSocketClientandISpeakWebSocketClientinterfaces, clarifying their functionality.Bug Fixes
Clientclass.Refactor