Reorganize Factory, Add CancelToken Option to Send Functions #347
Conversation
|
Warning Rate limit exceeded@dvonthenen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request includes updates across multiple files in the Deepgram library, focusing on enhancing logging, error handling, and method signatures to support cancellation tokens. Key modifications are made to 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: 5
🧹 Outside diff range and nitpick comments (17)
examples/speech-to-text/websocket/microphone/Program.cs (1)
Line range hint
103-104: Consider demonstrating CancelToken usage with microphone streaming.Since this PR introduces CancelToken support for Send functions, this example would be an ideal place to demonstrate proper cancellation handling during microphone streaming. This would help users understand how to implement graceful cancellation in their applications.
Consider updating the microphone streaming section to demonstrate CancelToken usage:
// Microphone streaming -var microphone = new Microphone(liveClient.Send); +using var cts = new CancellationTokenSource(); +var microphone = new Microphone((data) => liveClient.SendImmediately(data, cts.Token)); microphone.Start(); // Wait for the user to press a key Console.ReadKey(); +cts.Cancel(); // Signal cancellation before stopping // Stop the microphone microphone.Stop();Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (2)
114-115: Fix documentation and parameter naming.
- Remove the extra forward slash in the XML documentation.
- Consider renaming
_cancellationTokentocancellationTokenas underscore prefix is typically used for private fields.Apply this diff:
- /// /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> - public Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null); + /// <param name="cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> + public Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null);
122-123: Fix documentation and parameter naming (same as SendBinaryImmediately).
- Remove the extra forward slash in the XML documentation.
- Consider renaming
_cancellationTokentocancellationTokenas underscore prefix is typically used for private fields.Apply this diff:
- /// /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> - public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null); + /// <param name="cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> + public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null);Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (1)
144-145: Fix parameter naming and XML comment formatting.
- The parameter name
_cancellationTokenuses an underscore prefix, which typically denotes private fields in C#. Consider renaming tocancellationToken.- There's a double forward slash in the XML comment.
- /// /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> - public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null); + /// <param name="cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> + public Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null);examples/text-to-speech/websocket/simple/Program.cs (2)
Line range hint
83-106: Consider using async file operations.Since this is running in an async context, consider using async file operations for better scalability:
- using (BinaryWriter writer = new BinaryWriter(File.Open("output.wav", FileMode.Append))) - { - writer.Write(wavHeader); - appendWavHeader = false; - } + await using var stream = new FileStream("output.wav", FileMode.Append); + await stream.WriteAsync(wavHeader); + appendWavHeader = false;
Line range hint
108-117: Make file path configurable and use async operations.The hardcoded output path and synchronous file operations could be improved:
- Make the output path configurable:
+ private const string DefaultOutputPath = "output.wav"; + private static string OutputPath { get; set; } = DefaultOutputPath;
- Use async file operations:
- using (BinaryWriter writer = new BinaryWriter(File.Open("output.wav", FileMode.Append))) - { - writer.Write(e.Stream.ToArray()); - } + await using var stream = new FileStream(OutputPath, FileMode.Append); + await stream.WriteAsync(e.Stream.ToArray());Deepgram/Clients/Speak/v2/WebSocket/Client.cs (1)
330-340: Fix parameter naming convention.The parameter name
_cancellationTokenstarts with an underscore, which typically indicates a private field. Consider renaming it to follow C# parameter naming conventions.- public override async Task SendClose(bool nullByte = false, CancellationTokenSource? _cancellationToken = null) + public override async Task SendClose(bool nullByte = false, CancellationTokenSource? cancellationToken = null) { if (_clientWebSocket == null || !IsConnected()) { Log.Warning("SendClose", "ClientWebSocket is null or not connected. Skipping..."); return; } - var _cancelToken = _cancellationToken ?? _cancellationTokenSource; + var cancelToken = cancellationToken ?? _cancellationTokenSource;Deepgram/Abstractions/v2/AbstractWebSocketClient.cs (1)
Line range hint
509-524: Consider enhancing JSON parsing error handling.The current catch block for JsonException could provide more specific error information to help diagnose parsing issues. Consider including the raw response in the error log when JSON parsing fails.
catch (JsonException ex) { - Log.Error("ProcessTextMessage", $"{ex.GetType()} thrown {ex.Message}"); + Log.Error("ProcessTextMessage", $"Failed to parse JSON response: {ex.Message}"); + Log.Error("ProcessTextMessage", $"Raw response: {response}"); Log.Verbose("ProcessTextMessage", $"Exception: {ex}"); Log.Verbose("AbstractWebSocketClient.ProcessTextMessage", "LEAVE"); }Deepgram/Abstractions/v2/AbstractRestClient.cs (5)
Line range hint
1-39: Consider reducing logging of potentially sensitive configuration details.The constructor logs API version and base address which could expose sensitive information in production environments. Consider logging these only at TRACE level or masking sensitive parts of the URL.
- Log.Debug("AbstractRestClient", $"APIVersion: {options.APIVersion}"); - Log.Debug("AbstractRestClient", $"BaseAddress: {options.BaseAddress}"); + Log.Trace("AbstractRestClient", $"APIVersion: {options.APIVersion}"); + Log.Trace("AbstractRestClient", $"BaseAddress: {options.BaseAddress}");
Line range hint
40-600: Fix typo in exception logging across methods.There's a consistent typo in exception logging across all methods where "Exception" is spelled as "Excepton".
Example fix for one occurrence:
- Log.Verbose("GetAsync<T>", $"Excepton: {ex}"); + Log.Verbose("GetAsync<T>", $"Exception: {ex}");
Line range hint
164-277: Ensure proper disposal of MemoryStream in PostRetrieveLocalFileAsync.The method creates a new MemoryStream but doesn't handle its disposal. Consider wrapping it in a using statement or ensuring it's disposed properly by the caller.
- MemoryStream stream = new MemoryStream(); - await response.Content.CopyToAsync(stream); + var stream = new MemoryStream(); + try + { + await response.Content.CopyToAsync(stream); + stream.Position = 0; + return new LocalFileWithMetadata() + { + Metadata = result, + Content = stream, + }; + } + catch + { + stream.Dispose(); + throw; + }
Line range hint
601-642: Consider adding specific exception types for different error scenarios.The ThrowException method currently throws either DeepgramRESTException or DeepgramException. Consider creating more specific exception types for different HTTP status codes or error scenarios to help consumers handle errors more precisely.
Example implementation:
private static async Task ThrowException(string module, HttpResponseMessage response, string errMsg) { switch (response.StatusCode) { case HttpStatusCode.Unauthorized: throw new DeepgramAuthenticationException(errMsg); case HttpStatusCode.BadRequest: throw new DeepgramValidationException(errMsg); case HttpStatusCode.TooManyRequests: throw new DeepgramRateLimitException(errMsg); default: // existing implementation } }
Line range hint
40-600: Implement consistent security handling for sensitive headers across all methods.The PutAsync method correctly filters sensitive headers (password, token, authorization) from logging, but this security measure is not implemented in other HTTP methods. This inconsistency could lead to sensitive information being exposed in logs.
Consider extracting the header filtering logic to a shared method:
private bool IsSensitiveHeader(string headerKey) { var tmp = headerKey.ToLower(); return tmp.Contains("password") || tmp.Contains("token") || tmp.Contains("authorization") || tmp.Contains("auth"); }Apply this to all methods when logging headers:
if (headers != null) { foreach (var header in headers) { - Log.Debug("DeleteAsync<T>", $"Add Header {header.Key}={header.Value}"); + if (!IsSensitiveHeader(header.Key)) + { + Log.Debug("DeleteAsync<T>", $"Add Header {header.Key}={header.Value}"); + } request.Headers.Add(header.Key, header.Value); } }Deepgram/ClientFactory.cs (2)
82-90: Clarify XML documentation commentsThe XML documentation for the
CreateListenRESTClientmethod should provide more details about the method's purpose and usage. This will help developers understand how to utilize this method effectively.
94-102: Enhance documentation forCreateSpeakRESTClientSimilarly, the documentation comments for
CreateSpeakRESTClientcan be expanded to include information about when and how to use this method, as well as any important considerations.Deepgram/Clients/Listen/v2/WebSocket/Client.cs (2)
305-306: Rename local variable to avoid confusion with class member.The local variable
_cancelTokenis similar in name to the class member_cancellationTokenSource. To enhance code readability and prevent confusion, consider renaming the local variable tocancellationTokenor another distinct name.Apply this diff to rename the variable:
- var _cancelToken = _cancellationToken ?? _cancellationTokenSource; + var cancellationToken = _cancellationToken ?? _cancellationTokenSource;And update its usage accordingly:
- await _mutexSend.WaitAsync(_cancelToken.Token); + await _mutexSend.WaitAsync(cancellationToken.Token);
297-307: Align with existing method patterns regarding cancellation tokens.In the
retrieved_learnings, it was noted that methods likeSendKeepAlive,SendFinalize, andSendClosedo not requireCancellationTokenparameters because they operate on an internal queue and manage their own cancellation logic. Consistency across these methods improves maintainability.Consider reviewing other similar methods to ensure a consistent approach to cancellation token usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
Deepgram/Abstractions/v2/AbstractRestClient.cs(1 hunks)Deepgram/Abstractions/v2/AbstractWebSocketClient.cs(5 hunks)Deepgram/Abstractions/v2/Utilities.cs(1 hunks)Deepgram/Abstractions/v2/WebSocketMessage.cs(1 hunks)Deepgram/ClientFactory.cs(3 hunks)Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs(2 hunks)Deepgram/Clients/Interfaces/v2/ILiveClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/IOnPremClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/IPreRecordedClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/ISpeakClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs(3 hunks)Deepgram/Clients/Listen/v2/WebSocket/Client.cs(2 hunks)Deepgram/Clients/Listen/v2/WebSocket/ResponseEvent.cs(1 hunks)Deepgram/Clients/Listen/v2/WebSocket/UriSegments.cs(1 hunks)Deepgram/Clients/Speak/v2/WebSocket/Client.cs(2 hunks)Deepgram/Models/Common/v2/WebSocket/WebSocketType.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Alternative.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Average.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Channel.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Hit.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ListenType.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/LiveSchema.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Metadata.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/MetadataResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ModelInfo.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ResultResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Search.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/SpeechStartedResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/UtteranceEndResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Word.cs(1 hunks)examples/speech-to-text/websocket/microphone/Program.cs(1 hunks)examples/text-to-speech/websocket/simple/Program.cs(1 hunks)
💤 Files with no reviewable changes (4)
- Deepgram/Clients/Interfaces/v2/ILiveClient.cs
- Deepgram/Clients/Interfaces/v2/IOnPremClient.cs
- Deepgram/Clients/Interfaces/v2/IPreRecordedClient.cs
- Deepgram/Clients/Interfaces/v2/ISpeakClient.cs
✅ Files skipped from review due to trivial changes (19)
- Deepgram/Abstractions/v2/Utilities.cs
- Deepgram/Abstractions/v2/WebSocketMessage.cs
- Deepgram/Clients/Listen/v2/WebSocket/ResponseEvent.cs
- Deepgram/Clients/Listen/v2/WebSocket/UriSegments.cs
- Deepgram/Models/Common/v2/WebSocket/WebSocketType.cs
- Deepgram/Models/Listen/v2/WebSocket/Alternative.cs
- Deepgram/Models/Listen/v2/WebSocket/Average.cs
- Deepgram/Models/Listen/v2/WebSocket/Channel.cs
- Deepgram/Models/Listen/v2/WebSocket/Hit.cs
- Deepgram/Models/Listen/v2/WebSocket/ListenType.cs
- Deepgram/Models/Listen/v2/WebSocket/LiveSchema.cs
- Deepgram/Models/Listen/v2/WebSocket/Metadata.cs
- Deepgram/Models/Listen/v2/WebSocket/MetadataResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/ModelInfo.cs
- Deepgram/Models/Listen/v2/WebSocket/ResultResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/Search.cs
- Deepgram/Models/Listen/v2/WebSocket/SpeechStartedResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/UtteranceEndResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/Word.cs
🧰 Additional context used
📓 Learnings (4)
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (1)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (5)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs:106-107
Timestamp: 2024-10-28T18:22:59.455Z
Learning: In `Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs`, commented-out methods related to binary send, such as `SpeakWithStream`, `SendBinary`, and `SendBinaryImmediately`, are intentionally left in the code to indicate that binary send is not currently supported.
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/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
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.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs:85-85
Timestamp: 2024-10-28T18:23:10.963Z
Learning: In the `ISpeakWebSocketClient` interface, the method `SpeakWithText(string data)` is intentionally named and should not be renamed.
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (1)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
Deepgram/Clients/Speak/v2/WebSocket/Client.cs (1)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
🪛 GitHub Check: test (8.0.x)
Deepgram/ClientFactory.cs
[warning] 156-156:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: test (7.0.x)
Deepgram/ClientFactory.cs
[warning] 156-156:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: test (6.0.x)
Deepgram/ClientFactory.cs
[warning] 156-156:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: build
Deepgram/ClientFactory.cs
[warning] 156-156:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🔇 Additional comments (13)
examples/speech-to-text/websocket/microphone/Program.cs (1)
20-20: LGTM! Clear logging initialization example.
The change simplifies the default initialization while preserving documentation about available log levels as comments. This makes the example more approachable while still informing users about debugging options.
Also applies to: 22-22
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (2)
87-87: Consider using CancellationToken instead of CancellationTokenSource.
The interface currently uses CancellationTokenSource? for cancellation support. This is unusual as most .NET APIs accept CancellationToken instead. The key differences are:
CancellationTokenSourceis for creating and managing tokensCancellationTokenis for passing cancellation support
Consider changing all occurrences to use CancellationToken for consistency with .NET conventions.
Let's verify the usage pattern in the .NET SDK:
Also applies to: 115-115, 123-123
87-87: Reconsider adding CancellationToken to SendClose.
Based on previous discussions in PR #345, SendClose sends messages on an internal queue and will error/discard if the connection is invalid. Adding a CancellationToken parameter may not provide any practical benefit in this context.
Let's verify the internal queue implementation:
Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs (1)
111-111: Verify necessity of CancellationToken for SendClose.
Based on previous discussions, methods like SendClose that operate on an internal queue will error or be discarded if the connection is invalid. Consider if adding the CancellationTokenSource parameter provides meaningful value in this context.
✅ Verification successful
CancellationToken is justified for SendClose method
The implementation shows that SendClose performs async operations that benefit from cancellation support:
- It uses
_mutexSend.WaitAsync(_cancelToken.Token)for synchronization - It's used within the
Stopmethod which accepts a cancellation token - The method is part of the graceful shutdown process where cancellation control is valuable
The addition of the CancellationTokenSource parameter aligns with the codebase's pattern of providing cancellation control for async operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SendClose implementations to understand usage patterns
ast-grep --pattern 'SendClose($$$)'
Length of output: 323
Script:
#!/bin/bash
# Let's examine the implementation context of SendClose to understand its behavior
rg "SendClose" -B 5 -A 5
Length of output: 10976
examples/text-to-speech/websocket/simple/Program.cs (2)
20-22: LGTM! Appropriate logging configuration.
The change to use default logging level (Info) instead of verbose logging is a good practice for example code, as it provides a better out-of-the-box experience while still maintaining the option to enable verbose logging when needed.
Line range hint 147-157: Add CancelToken support for consistency with PR objectives.
The PR objectives mention adding cancel token support. Consider adding it to this example:
+ using System.Threading;
class Program
{
- static async Task Main(string[] args)
+ static async Task Main(string[] args)
+ {
+ using var cts = new CancellationTokenSource();
+ Console.CancelKeyPress += (s, e) => {
+ e.Cancel = true;
+ cts.Cancel();
+ };
+ await MainAsync(args, cts.Token);
+ }
+
+ static async Task MainAsync(string[] args, CancellationToken cancellationToken)
{
try
{Then update the WebSocket operations to use the token:
- bool bConnected = await speakClient.Connect(speakSchema);
+ bool bConnected = await speakClient.Connect(speakSchema, cancellationToken);Deepgram/Clients/Speak/v2/WebSocket/Client.cs (1)
Line range hint 345-360: LGTM! Implementation correctly handles cancellation tokens.
The implementation properly uses the cancellation token for mutex operations and message sending. The code is well-structured with appropriate error handling and resource cleanup.
Deepgram/Abstractions/v2/AbstractWebSocketClient.cs (3)
1-2: LGTM: Copyright notice updated correctly.
The copyright notice has been updated to reflect the current year 2024.
274-275: LGTM: Improved cancellation handling in SendBinaryImmediately.
The addition of optional CancellationTokenSource and proper token selection logic enhances the control over cancellation scenarios.
Also applies to: 283-285
622-622: LGTM: Improved shutdown handling.
The Stop method now properly propagates the cancellation token to SendClose, enhancing control over the shutdown process.
Deepgram/ClientFactory.cs (1)
111-114: Verify return type consistency for CreateSpeakWebSocketClient
The method CreateSpeakWebSocketClient returns a V2.ISpeakWebSocketClient, but instantiates SpeakWebSocketClient. Ensure that SpeakWebSocketClient correctly implements V2.ISpeakWebSocketClient to maintain type consistency.
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (2)
297-307:
Consider removing the _cancellationToken parameter from SendClose.
Based on previous learnings, adding a CancellationTokenSource parameter to the SendClose method may not be necessary. Since SendClose sends messages on an internal queue and will error or be discarded if the connection is no longer valid, introducing this parameter might add unnecessary complexity.
I suggest reverting the method signature to its original form:
-public override async Task SendClose(bool nullByte = false, CancellationTokenSource? _cancellationToken = null)
+public override async Task SendClose(bool nullByte = false)Would you like assistance in updating the method accordingly?
305-306:
Ensure consistent usage of the cancellation token within SendClose.
If you decide to retain the _cancellationToken parameter, make sure to use the derived cancellation token consistently throughout the method. Currently, _cancellationTokenSource.Token is used instead of _cancelToken.Token, which could lead to unintended behavior if a different cancellation token is provided.
Apply this diff to correct the cancellation token usage:
- await _clientWebSocket.SendAsync(new ArraySegment<byte>(new byte[1] { 0 }), WebSocketMessageType.Binary, true, _cancellationTokenSource.Token)
+ await _clientWebSocket.SendAsync(new ArraySegment<byte>(new byte[1] { 0 }), WebSocketMessageType.Binary, true, _cancelToken.Token)Likely invalid or redundant comment.
davidvonthenen
force-pushed
the
organization-change-for-v2
branch
2 times, most recently
from
8fa5030
to
9e549a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (3)
114-115: Fix documentation format.The XML documentation has an extra set of forward slashes.
- /// /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> + /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param>
122-123: Fix documentation format.The XML documentation has an extra set of forward slashes.
- /// /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param> + /// <param name="_cancellationToken">Provide a cancel token to be used for the send function or use the internal one</param>
87-87: Consider renaming cancellation token parameters.The parameter name
_cancellationTokenuses an underscore prefix, which typically indicates a private field. Consider renaming tocancellationTokento follow C# parameter naming conventions.- public Task SendClose(bool nullByte = false, CancellationTokenSource? _cancellationToken = null); + public Task SendClose(bool nullByte = false, CancellationTokenSource? cancellationToken = null); - 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);Also applies to: 115-115, 123-123
Deepgram/ClientFactory.cs (2)
90-94: Documentation needs update.The XML documentation comment incorrectly states this is for creating an "AnalyzeClient" when it actually creates a "SpeakWebSocketClient".
Apply this diff to fix the documentation:
/// <summary> - /// Create a new AnalyzeClient + /// Create a new SpeakWebSocketClient /// </summary> /// <param name="apiKey"></param> /// <param name="options"></param>
113-114: Consider implementing reflection-based client instantiation.Multiple TODO comments indicate a plan to use reflection for client instantiation. This could be implemented as a generic method to reduce code duplication across all version-specific factory methods.
Would you like me to propose a generic reflection-based implementation that could handle all client types?
Also applies to: 133-134, 153-154, 163-164, 173-174, 183-184
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (1)
Line range hint
297-326: Review CancellationToken implementation in SendClose.Several issues need attention:
- According to previous feedback, SendClose doesn't require a CancellationToken as messages are discarded if the connection is invalid
- Inconsistent token usage: WaitAsync uses
_cancelToken.Tokenwhile SendAsync uses_cancellationTokenSource.Token- Parameter naming convention: Parameters shouldn't start with underscore
Consider these changes:
-public override async Task SendClose(bool nullByte = false, CancellationTokenSource? _cancellationToken = null) +public override async Task SendClose(bool nullByte = false) { if (_clientWebSocket == null || !IsConnected()) { Log.Warning("SendClose", "ClientWebSocket is null or not connected. Skipping..."); return; } - // provide a cancellation token, or use the one in the class - var _cancelToken = _cancellationToken ?? _cancellationTokenSource; - Log.Debug("SendClose", "Sending Close Message Immediately..."); if (nullByte) { // send a close to Deepgram - await _mutexSend.WaitAsync(_cancelToken.Token); + await _mutexSend.WaitAsync(_cancellationTokenSource.Token); try { await _clientWebSocket.SendAsync(new ArraySegment<byte>(new byte[1] { 0 }), WebSocketMessageType.Binary, true, _cancellationTokenSource.Token) .ConfigureAwait(false);Deepgram/Abstractions/v2/AbstractWebSocketClient.cs (2)
274-274: Remove the extraneous comma in the XML documentation comment.The comma at the end of the summary tag is unnecessary and may cause issues during documentation generation.
Suggested fix:
- /// </summary>, + /// </summary>
275-287: Rename parameters and variables to follow C# naming conventions.Parameters and local variables should not start with underscores. Leading underscores are typically reserved for private instance fields. Consider renaming
_cancellationTokentocancellationTokenand_cancelTokentocancelTokenfor clarity and to adhere to convention.Suggested fix for
SendBinaryImmediatelymethod:-public virtual async Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null) +public virtual async Task SendBinaryImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null)And in the method body:
- var _cancelToken = _cancellationToken ?? _cancellationTokenSource; + var cancelToken = cancellationToken ?? _cancellationTokenSource;Similarly, for the
SendMessageImmediatelymethod:-public virtual async Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? _cancellationToken = null) +public virtual async Task SendMessageImmediately(byte[] data, int length = Constants.UseArrayLengthForSend, CancellationTokenSource? cancellationToken = null)And in the method body:
- var _cancelToken = _cancellationToken ?? _cancellationTokenSource; + var cancelToken = cancellationToken ?? _cancellationTokenSource;Also applies to: 306-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
Deepgram/Abstractions/v2/AbstractRestClient.cs(1 hunks)Deepgram/Abstractions/v2/AbstractWebSocketClient.cs(5 hunks)Deepgram/Abstractions/v2/Utilities.cs(1 hunks)Deepgram/Abstractions/v2/WebSocketMessage.cs(1 hunks)Deepgram/ClientFactory.cs(3 hunks)Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs(2 hunks)Deepgram/Clients/Interfaces/v2/ILiveClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/IOnPremClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/IPreRecordedClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/ISpeakClient.cs(0 hunks)Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs(2 hunks)Deepgram/Clients/Listen/v2/WebSocket/Client.cs(2 hunks)Deepgram/Clients/Listen/v2/WebSocket/ResponseEvent.cs(1 hunks)Deepgram/Clients/Listen/v2/WebSocket/UriSegments.cs(1 hunks)Deepgram/Clients/Speak/v2/WebSocket/Client.cs(2 hunks)Deepgram/Models/Common/v2/WebSocket/WebSocketType.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Alternative.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Average.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Channel.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Hit.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ListenType.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/LiveSchema.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Metadata.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/MetadataResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ModelInfo.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/ResultResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Search.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/SpeechStartedResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/UtteranceEndResponse.cs(1 hunks)Deepgram/Models/Listen/v2/WebSocket/Word.cs(1 hunks)examples/speech-to-text/websocket/microphone/Program.cs(1 hunks)examples/text-to-speech/websocket/simple/Program.cs(1 hunks)
💤 Files with no reviewable changes (4)
- Deepgram/Clients/Interfaces/v2/ILiveClient.cs
- Deepgram/Clients/Interfaces/v2/IOnPremClient.cs
- Deepgram/Clients/Interfaces/v2/IPreRecordedClient.cs
- Deepgram/Clients/Interfaces/v2/ISpeakClient.cs
🚧 Files skipped from review as they are similar to previous changes (24)
- Deepgram/Abstractions/v2/AbstractRestClient.cs
- Deepgram/Abstractions/v2/Utilities.cs
- Deepgram/Abstractions/v2/WebSocketMessage.cs
- Deepgram/Clients/Interfaces/v2/ISpeakWebSocketClient.cs
- Deepgram/Clients/Listen/v2/WebSocket/ResponseEvent.cs
- Deepgram/Clients/Listen/v2/WebSocket/UriSegments.cs
- Deepgram/Clients/Speak/v2/WebSocket/Client.cs
- Deepgram/Models/Common/v2/WebSocket/WebSocketType.cs
- Deepgram/Models/Listen/v2/WebSocket/Alternative.cs
- Deepgram/Models/Listen/v2/WebSocket/Average.cs
- Deepgram/Models/Listen/v2/WebSocket/Channel.cs
- Deepgram/Models/Listen/v2/WebSocket/Hit.cs
- Deepgram/Models/Listen/v2/WebSocket/ListenType.cs
- Deepgram/Models/Listen/v2/WebSocket/LiveSchema.cs
- Deepgram/Models/Listen/v2/WebSocket/Metadata.cs
- Deepgram/Models/Listen/v2/WebSocket/MetadataResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/ModelInfo.cs
- Deepgram/Models/Listen/v2/WebSocket/ResultResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/Search.cs
- Deepgram/Models/Listen/v2/WebSocket/SpeechStartedResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/UtteranceEndResponse.cs
- Deepgram/Models/Listen/v2/WebSocket/Word.cs
- examples/speech-to-text/websocket/microphone/Program.cs
- examples/text-to-speech/websocket/simple/Program.cs
🧰 Additional context used
📓 Learnings (3)
Deepgram/ClientFactory.cs (5)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#347
File: Deepgram/ClientFactory.cs:146-161
Timestamp: 2024-11-01T16:17:11.630Z
Learning: In `Deepgram/ClientFactory.cs`, the method `CreateListenWebSocketClient(int version, ...)` is intentionally returning `object` as a placeholder because there is only one version of the client. It should not be suggested to change the return type until multiple versions are implemented.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#347
File: Deepgram/ClientFactory.cs:141-143
Timestamp: 2024-11-01T16:20:16.878Z
Learning: In `Deepgram/ClientFactory.cs`, the use of `ListenV1.Client` is intentional for backward compatibility and should not be suggested for replacement with `ListenV2.Client` in code reviews.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#347
File: Deepgram/ClientFactory.cs:210-261
Timestamp: 2024-11-01T16:16:34.733Z
Learning: In `Deepgram/ClientFactory.cs`, the deprecated methods `CreateSpeakClient`, `CreatePreRecordedClient`, and `CreateOnPremClient` are intentionally retained for backward compatibility and should not be suggested for removal in code reviews.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#310
File: Deepgram/ClientFactory.cs:63-72
Timestamp: 2024-10-09T02:19:46.087Z
Learning: The `CreateOnPremClient` method in `Deepgram/ClientFactory.cs` is intentionally retained to allow users time to migrate to the new `CreateSelfHostedClient` method. This strategy ensures backward compatibility and a smooth transition for users.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#310
File: Deepgram/ClientFactory.cs:63-72
Timestamp: 2024-06-28T17:26:13.034Z
Learning: The `CreateOnPremClient` method in `Deepgram/ClientFactory.cs` is intentionally retained to allow users time to migrate to the new `CreateSelfHostedClient` method. This strategy ensures backward compatibility and a smooth transition for users.
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (2)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenRESTClient.cs:23-23
Timestamp: 2024-10-28T19:00:16.518Z
Learning: In this codebase, methods intentionally accept `CancellationTokenSource` parameters to implement per function timeouts, so do not suggest replacing them with `CancellationToken`.
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (1)
Learnt from: dvonthenen
PR: deepgram/deepgram-dotnet-sdk#345
File: Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs:77-77
Timestamp: 2024-10-28T19:04:15.779Z
Learning: In `IListenWebSocketClient`, the methods `SendKeepAlive`, `SendFinalize`, and `SendClose` send messages on an internal queue and will error or be discarded if the connection is no longer valid, so adding `CancellationToken` parameters is not necessary.
🪛 GitHub Check: test (8.0.x)
Deepgram/ClientFactory.cs
[warning] 143-143:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: test (7.0.x)
Deepgram/ClientFactory.cs
[warning] 143-143:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: test (6.0.x)
Deepgram/ClientFactory.cs
[warning] 143-143:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🪛 GitHub Check: build
Deepgram/ClientFactory.cs
[warning] 143-143:
'Client' is obsolete: 'Please use Deepgram.Clients.Listen.v2.WebSocket instead'
🔇 Additional comments (6)
Deepgram/Clients/Interfaces/v2/IListenWebSocketClient.cs (1)
87-87: Reconsider adding CancellationTokenSource to SendClose.
Based on previous feedback in PR #345, the SendClose method sends messages on an internal queue and will error or be discarded if the connection is no longer valid, making the CancellationTokenSource parameter unnecessary.
Let's verify the internal queue implementation:
Deepgram/ClientFactory.cs (2)
66-87: LGTM! Well-structured REST client factory methods.
The new REST client factory methods are well-documented and follow consistent patterns in terms of parameter ordering and default values.
201-268: LGTM! Well-documented deprecation notices.
The deprecated methods section is properly marked with [Obsolete] attributes and includes comprehensive warning comments with clear migration paths.
Deepgram/Clients/Listen/v2/WebSocket/Client.cs (2)
1-1: LGTM: Copyright notice updated correctly.
The copyright year has been updated to 2024 as intended.
Line range hint 332-399: LGTM: Well-structured logging implementation.
The logging enhancements in ProcessKeepAlive follow best practices:
- Consistent entry/exit logging at Verbose level
- Appropriate use of log levels (Debug for flow, Warning for issues)
- Detailed exception logging with stack traces at Verbose level
Deepgram/Abstractions/v2/AbstractWebSocketClient.cs (1)
622-622: Verify the use of cancelToken in the Stop method.
Ensure that cancelToken is appropriately utilized in all asynchronous calls within the Stop method to support proper cancellation behavior.
davidvonthenen
force-pushed
the
organization-change-for-v2
branch
from
4539733
to
b52d203
Compare
Proposed changes
This just:
v2clients onlyTested using Listen and Speak WS Clients examples.
Types 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
Bug Fixes
Chores
Documentation