Fix Issue 264 - Using Shared WebSocket Class Implementation

[dvonthenen]

Proposed changes

Addresses: #264

This is to make way to support Agent API in a much cleaner and reusable way. It should drastically decrease code spawl.

Tested all Speech-to-Text and Text-to-Speech examples. Basically, anything that uses a WebSocket.

Also, unit tests passed further demonstrating that users should be unaware of this internal change.

Types of changes

What types of changes does your code introduce to the community Python SDK?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update or tests (if none of the other choices apply)

Checklist

Put an x in 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.

• I have read the CONTRIBUTING doc
• I have lint'ed all of my code using repo standards
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

NA

Summary by CodeRabbit

• New Features

  • Introduced new abstract classes for managing WebSocket connections (AbstractAsyncWebSocketClient and AbstractSyncWebSocketClient).
  • Added new error handling classes: DeepgramApiError and DeepgramUnknownApiError.
  • Implemented signal handling for graceful shutdowns in the asynchronous text-to-speech application.

• Improvements

  • Enhanced error handling in WebSocket operations.
  • Improved type safety and clarity in class attributes.
  • Streamlined import statements for better readability and maintainability across various modules.

• Bug Fixes

  • Added checks in thread management to prevent runtime errors.

[coderabbitai]

Caution

Review failed

The head commit changed during the review from e0ad551 to 4c9c639.

Walkthrough

The changes involve enhancements to the deepgram library, focusing on optional typing for class attributes, improved error handling in WebSocket connections, and the introduction of new abstract classes for managing WebSocket communications. The modifications also include restructured import statements for better organization and clarity across various modules. New exception classes have been added to handle specific API errors, enhancing the overall robustness of the error management system.

Changes

Files	Change Summary
deepgram/audio/speaker/speaker.py	Introduced optional typing for class attributes and enhanced error handling in _start_asyncio_receiver. Updated finish method to check for thread existence before joining.
deepgram/clients/__init__.py	Reorganized import statements for better readability.
deepgram/clients/analyze/v1/async_client.py	Modified import paths for AbstractAsyncRestClient, DeepgramError, and DeepgramTypeError to simplify the module structure.
deepgram/clients/analyze/v1/client.py	Changed import paths for AbstractSyncRestClient, DeepgramError, and DeepgramTypeError to improve clarity and maintainability.
deepgram/clients/common/__init__.py	Added new imports for DeepgramApiError, DeepgramUnknownApiError, and several abstract client classes, enhancing error handling and client abstractions.
deepgram/clients/common/v1/__init__.py	Introduced imports for new error classes and abstract client classes, expanding functionality.
deepgram/clients/common/v1/abstract_async_websocket.py	Added AbstractAsyncWebSocketClient class with methods for managing WebSocket connections asynchronously, including error handling and message processing.
deepgram/clients/common/v1/abstract_sync_websocket.py	Introduced AbstractSyncWebSocketClient class for synchronous WebSocket management, featuring methods for connection handling and message processing.
deepgram/clients/common/v1/errors.py	Added DeepgramApiError and DeepgramUnknownApiError exception classes to handle specific API-related errors.
deepgram/clients/listen/v1/rest/async_client.py	Updated import paths for AbstractAsyncRestClient, DeepgramError, and DeepgramTypeError for consistency.
deepgram/clients/listen/v1/rest/client.py	Changed import paths for AbstractSyncRestClient, DeepgramError, and DeepgramTypeError to streamline imports.
deepgram/clients/listen/v1/websocket/async_client.py	Refactored AsyncListenWebSocketClient to inherit from AbstractAsyncWebSocketClient, consolidating functionality and simplifying error handling.
deepgram/clients/listen/v1/websocket/client.py	Refactored ListenWebSocketClient to inherit from AbstractSyncWebSocketClient, improving structure and message handling.
deepgram/clients/manage/v1/async_client.py	Updated import path for AbstractAsyncRestClient to a common module.
deepgram/clients/manage/v1/client.py	Changed import path for AbstractSyncRestClient to a common module.
deepgram/clients/selfhosted/v1/async_client.py	Updated import path for AbstractAsyncRestClient to a common module.
deepgram/clients/selfhosted/v1/client.py	Changed import path for AbstractSyncRestClient to a common module.
deepgram/clients/speak/v1/rest/async_client.py	Modified import paths for AbstractAsyncRestClient, DeepgramError, and DeepgramTypeError for consistency.
deepgram/clients/speak/v1/rest/client.py	Changed import paths for AbstractSyncRestClient, DeepgramError, and DeepgramTypeError to streamline imports.
deepgram/clients/speak/v1/websocket/async_client.py	Refactored AsyncSpeakWSClient to inherit from AbstractAsyncWebSocketClient, enhancing structure and functionality.
deepgram/clients/speak/v1/websocket/client.py	Refactored SpeakWSClient to inherit from AbstractSyncWebSocketClient, improving structure and message handling.
examples/text-to-speech/websocket/async_complete/main.py	Introduced signal handling for graceful shutdowns and a global variable to control message printing.
examples/text-to-speech/websocket/complete/main.py	Added a global variable to manage message printing in the on_binary_data method and modified logging configuration.

Possibly related PRs

• Re-expose Error Classes #462: This PR focuses on re-exposing error classes, which aligns with the main PR's improvements in error handling within the _start_asyncio_receiver method, enhancing robustness in connection management.

Suggested reviewers

• jpvajda
• SandraRodgers
• lukeocodes

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[dvonthenen]

Unit Tests passed.

[coderabbitai]

Actionable comments posted: 32

🧹 Outside diff range and nitpick comments (22)

deepgram/clients/common/v1/websocket_events.py (2)

8-13: LGTM: Class definition and docstring are well-structured.

The WebSocketEvents class is correctly defined as a subclass of StrEnum, and the docstring provides a clear description of the class purpose.

Consider removing the unnecessary empty line (line 9) before the class definition to improve code consistency.

---

15-19: LGTM: Enum members are well-defined.

The enum members cover the basic events expected in WebSocket communication, and the use of string values matching the member names is consistent and follows good practices for string enums.

Consider adding a "Message" event if it's relevant to the Deepgram API. This would typically represent received messages from the WebSocket connection. For example:

Message: str = "Message"

This addition would make the enum more comprehensive for typical WebSocket interactions.

deepgram/clients/common/v1/__init__.py (1)

13-16: LGTM! Improved client architecture with abstract base classes.

The addition of abstract client classes (AbstractAsyncRestClient, AbstractSyncRestClient, AbstractAsyncWebSocketClient, AbstractSyncWebSocketClient) is an excellent enhancement. This change aligns perfectly with the PR objective of organizing API support in a more reusable manner and will likely contribute significantly to reducing code sprawl.

The separation of sync/async and REST/WebSocket clients indicates a well-structured design that should improve maintainability and extensibility.

Consider grouping related imports together for better readability. For example:

from .abstract_async_rest import AbstractAsyncRestClient
from .abstract_sync_rest import AbstractSyncRestClient
from .abstract_async_websocket import AbstractAsyncWebSocketClient
from .abstract_sync_websocket import AbstractSyncWebSocketClient

This minor reorganization could make the imports easier to scan and understand at a glance.

deepgram/clients/common/v1/helpers.py (2)

11-32: LGTM: Well-implemented function with proper error handling.

The append_query_params function is well-implemented, handling various data types and edge cases correctly. It follows Python best practices and includes appropriate type hinting.

Consider adding a type hint for the return value to improve code clarity:

-def append_query_params(url: str, params: Optional[Dict] = None):
+def append_query_params(url: str, params: Optional[Dict] = None) -> str:

---

36-53: LGTM: Well-implemented WebSocket URL conversion function.

The convert_to_websocket_url function effectively converts regular URLs to WebSocket URLs, handling both HTTP and HTTPS protocols correctly.

Consider the following improvements:

1. Add a return type hint:

-def convert_to_websocket_url(base_url: str, endpoint: str):
+def convert_to_websocket_url(base_url: str, endpoint: str) -> str:

2. Use str.startswith() instead of in for more precise protocol checking:

-        if "http://" in base_url:
+        if base_url.startswith("http://"):

3. Simplify the URL scheme replacement using str.replace():

-    if re.match(r"^https?://", base_url, re.IGNORECASE):
-        if base_url.startswith("http://"):
-            use_ssl = False  # Override to false if http:// is found
-        base_url = base_url.replace("https://", "").replace("http://", "")
+    if base_url.startswith(("http://", "https://")):
+        use_ssl = base_url.startswith("https://")
+        base_url = base_url.replace("http://", "").replace("https://", "")

These changes will improve code clarity and efficiency.

deepgram/clients/common/__init__.py (1)

12-13: LGTM: Abstract REST client imports look good

The addition of AbstractAsyncRestClient and AbstractSyncRestClient aligns with the PR objective of organizing the Agent API support in a more reusable manner. These abstract classes will provide a solid foundation for both asynchronous and synchronous REST API interactions.

Consider combining these imports with the previous import statement for consistency:

from .v1 import (
    DeepgramError,
    DeepgramTypeError,
    DeepgramApiError,
    DeepgramUnknownApiError,
    AbstractAsyncRestClient,
    AbstractSyncRestClient,
)

deepgram/clients/common/v1/errors.py (2)

40-58: LGTM! Consider adding type hints for better code clarity.

The DeepgramApiError class is well-implemented and follows best practices for custom exceptions. The docstring provides clear explanations, and the __str__ method offers a useful string representation.

Consider adding type hints to the __init__ method for improved code clarity:

-    def __init__(self, message: str, status: str, original_error=None):
+    def __init__(self, message: str, status: str, original_error: Optional[Dict[str, Any]] = None):

This change would require adding the following import at the top of the file:

from typing import Dict, Any, Optional

---

Line range hint 1-77: Consider standardizing error handling across all exception classes.

While the new exception classes (DeepgramApiError and DeepgramUnknownApiError) provide more detailed error information, it might be beneficial to standardize the error handling approach across all exception classes in this file for better consistency and maintainability.

Consider the following suggestions:

1. Add a status attribute to DeepgramError and DeepgramTypeError classes, similar to the new classes. This would allow for consistent error reporting across all Deepgram-related exceptions.

2. Standardize the __str__ method across all classes to include the status (if applicable) and maintain a consistent format.

3. Consider creating a base DeepgramBaseError class that implements common functionality, and have all other exception classes inherit from it. This would reduce code duplication and ensure consistency.

Here's a basic example of how this could look:

class DeepgramBaseError(Exception):
    def __init__(self, message: str, status: Optional[str] = None):
        super().__init__(message)
        self.name = self.__class__.__name__
        self.message = message
        self.status = status

    def __str__(self):
        return f"{self.name}: {self.message}" + (f" (Status: {self.status})" if self.status else "")

class DeepgramError(DeepgramBaseError):
    pass

class DeepgramTypeError(DeepgramBaseError):
    pass

class DeepgramApiError(DeepgramBaseError):
    def __init__(self, message: str, status: str, original_error: Optional[Dict[str, Any]] = None):
        super().__init__(message, status)
        self.original_error = original_error

class DeepgramUnknownApiError(DeepgramBaseError):
    pass

This structure would provide a more consistent and maintainable approach to error handling across the Deepgram SDK.

examples/text-to-speech/websocket/complete/main.py (1)

Line range hint 1-124: Summary of changes and suggestions for improvement

The changes in this file aim to improve the user experience by reducing repetitive warning messages. However, there are a few areas that could be further improved:

1. Replace the global variable usage with a more encapsulated approach, such as using class attributes or instance variables.
2. Provide clear explanations for the commented-out configuration options, either in the code comments or in the documentation.
3. Consider adding more comprehensive error handling and logging throughout the example to make it more robust and easier to debug.

These improvements would make the example more maintainable, easier to understand, and more aligned with best practices in Python development.

deepgram/audio/speaker/speaker.py (2)

29-44: LGTM: Improved type safety for class attributes

The changes to make _stream, _output_device_index, _thread, _receiver_thread, and _loop explicitly optional and initialized as None improve type safety and provide a clear initial state for these attributes. This aligns with best practices for type hinting in Python.

Consider adding type hints to the other class attributes (_audio, _chunk, _rate, _channels, _queue, _exit) for consistency and to further improve type safety throughout the class.

---

222-232: Improved WebSocket exception handling

The addition of specific exception handling for WebSocket-related errors (ConnectionClosedOK, ConnectionClosed, and WebSocketException) improves the robustness of the _start_asyncio_receiver method. This change provides more granular logging for different connection issues, which will aid in debugging and monitoring the connection state.

Consider replacing the broad except Exception clause at the end of the method with more specific exception types that you expect to encounter. This will make the error handling more precise and easier to maintain.

deepgram/clients/common/v1/abstract_async_rest.py (1)

12-13: LGTM! Consider using absolute imports for better maintainability.

The changes to the import statements are correct and don't affect the functionality of the AbstractAsyncRestClient class. The consolidation of error imports improves code readability.

Consider using absolute imports instead of relative imports for DeepgramClientOptions. This can make the code more maintainable and less prone to errors if the file structure changes in the future. For example:

from deepgram.options import DeepgramClientOptions

examples/text-to-speech/websocket/async_complete/main.py (1)

25-25: Use asyncio.get_running_loop() instead of asyncio.get_event_loop()

In asynchronous functions, asyncio.get_running_loop() is preferred over asyncio.get_event_loop() to get the currently running event loop.

Suggested change:

-loop = asyncio.get_event_loop()
+loop = asyncio.get_running_loop()

deepgram/clients/common/v1/abstract_async_websocket.py (1)

2-2: Typographical error in license statement.

In line 2, the phrase "governed by a MIT license" should use "an" instead of "a" because "MIT" starts with a vowel sound.

Apply this diff to correct the typo:

-# Use of this source code is governed by a MIT license that can be found in the LICENSE file.
+# Use of this source code is governed by an MIT license that can be found in the LICENSE file.

deepgram/clients/listen/v1/websocket/client.py (3)

248-248: Simplify event emission by using the enum directly

When emitting events, wrapping the event enum within itself is redundant. Use the enum member directly for clarity.

Apply this diff to simplify event emission:

-                            LiveTranscriptionEvents(LiveTranscriptionEvents.Open),
+                            LiveTranscriptionEvents.Open,

Repeat this change for all similar occurrences in the match statement cases.

---

205-205: Remove duplicate logging statements

The debug log self._logger.debug("callback handlers for: %s", event) is executed twice consecutively without any state change, which is redundant.

Remove one of the duplicate log statements:

            self._logger.debug("callback handlers for: %s", event)
-            self._logger.debug("callback handlers for: %s", event)

Also applies to: 212-212

---

509-511: Unused method _close_message

The _close_message method is defined but not called anywhere in the class. This could indicate dead code.

Verify whether this method is needed:

• If it is intended to be used, ensure it is called appropriately.
• If not needed, consider removing it to clean up the codebase.

deepgram/clients/listen/v1/websocket/async_client.py (1)

510-511: Potential Issue with _close_message Return Value

The _close_message method returns the result of await self.send(...). Ensure that this method's return value is appropriately handled wherever _close_message is called, and consider documenting what the return value represents.

deepgram/clients/speak/v1/websocket/client.py (2)

236-253: Redundant debug logging in _emit method

Multiple debug logs before and after iterating through threads may clutter the log output without providing significant value.

Consider condensing or removing some of the debug statements to streamline logging output.

---

Line range hint 513-551: Improve exception handling in send_raw method

Using except Exception as e may catch unexpected exceptions and make debugging challenging.

Catch specific exceptions that super().send(msg) might raise, such as network-related exceptions.

- except Exception as e:  # pylint: disable=broad-except
+ except WebSocketException as e:

Ensure relevant exceptions are imported and appropriately handled.

deepgram/clients/speak/v1/websocket/async_client.py (2)

Line range hint 363-379: Avoid catching broad exceptions in _process_text

Catching all exceptions can mask underlying issues. It's advisable to catch specific exceptions to enhance error handling and make debugging easier.

---

Line range hint 433-454: Avoid catching broad exceptions in _flush

Using a broad except Exception block may suppress critical errors. Consider handling specific exceptions to improve reliability and maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c2b0790 and b2b07b9.

📒 Files selected for processing (30)

• deepgram/audio/speaker/speaker.py (4 hunks)
• deepgram/clients/init.py (1 hunks)
• deepgram/clients/analyze/v1/async_client.py (1 hunks)
• deepgram/clients/analyze/v1/client.py (1 hunks)
• deepgram/clients/common/init.py (1 hunks)
• deepgram/clients/common/v1/init.py (1 hunks)
• deepgram/clients/common/v1/abstract_async_rest.py (1 hunks)
• deepgram/clients/common/v1/abstract_async_websocket.py (1 hunks)
• deepgram/clients/common/v1/abstract_sync_rest.py (1 hunks)
• deepgram/clients/common/v1/abstract_sync_websocket.py (1 hunks)
• deepgram/clients/common/v1/errors.py (1 hunks)
• deepgram/clients/common/v1/helpers.py (1 hunks)
• deepgram/clients/common/v1/websocket_events.py (1 hunks)
• deepgram/clients/errors.py (0 hunks)
• deepgram/clients/helpers.py (0 hunks)
• deepgram/clients/listen/v1/rest/async_client.py (1 hunks)
• deepgram/clients/listen/v1/rest/client.py (1 hunks)
• deepgram/clients/listen/v1/websocket/async_client.py (11 hunks)
• deepgram/clients/listen/v1/websocket/client.py (11 hunks)
• deepgram/clients/listen/v1/websocket/response.py (0 hunks)
• deepgram/clients/manage/v1/async_client.py (1 hunks)
• deepgram/clients/manage/v1/client.py (1 hunks)
• deepgram/clients/selfhosted/v1/async_client.py (1 hunks)
• deepgram/clients/selfhosted/v1/client.py (1 hunks)
• deepgram/clients/speak/v1/rest/async_client.py (1 hunks)
• deepgram/clients/speak/v1/rest/client.py (1 hunks)
• deepgram/clients/speak/v1/websocket/async_client.py (21 hunks)
• deepgram/clients/speak/v1/websocket/client.py (18 hunks)
• examples/text-to-speech/websocket/async_complete/main.py (4 hunks)
• examples/text-to-speech/websocket/complete/main.py (2 hunks)

💤 Files not reviewed due to no reviewable changes (3)

• deepgram/clients/errors.py
• deepgram/clients/helpers.py
• deepgram/clients/listen/v1/websocket/response.py

✅ Files skipped from review due to trivial changes (3)

• deepgram/clients/init.py
• deepgram/clients/manage/v1/async_client.py
• deepgram/clients/manage/v1/client.py

🔇 Additional comments (53)

deepgram/clients/common/v1/websocket_events.py (2)

1-3: LGTM: Copyright and license information is complete.

The copyright notice, license information, and SPDX identifier are present and up to date. This follows best practices for open-source software.

---

1-19: Summary: New WebSocketEvents class aligns well with PR objectives.

This new file introduces a well-structured WebSocketEvents class that enumerates possible events from the Deepgram API. The implementation follows good practices and provides a clear structure for handling WebSocket events.

The introduction of this class aligns well with the PR objectives of enhancing support for the Agent API in a more organized and reusable manner. It should contribute to reducing code sprawl within the deepgram-python-sdk by providing a centralized definition of WebSocket events.

Consider the suggestions made in the review comments to further improve the implementation:

1. Potentially use the standard library's enum module if the minimum Python version allows it.
2. Add a "Message" event if relevant to the Deepgram API.

Overall, this addition is a positive step towards improving the SDK's structure and maintainability.

deepgram/clients/common/v1/__init__.py (2)

7-12: LGTM! Enhanced error handling for API operations.

The addition of DeepgramApiError and DeepgramUnknownApiError to the imports from the .errors module is a positive change. This enhancement aligns well with the PR objective of improving API support and will likely contribute to more robust error handling in API operations.

---

7-16: Summary: Excellent enhancements to error handling and client architecture.

The changes in this file significantly improve the module's capabilities by introducing new error types and abstract client classes. These additions align perfectly with the PR objectives of enhancing API support and reducing code sprawl.

The new error classes (DeepgramApiError and DeepgramUnknownApiError) will allow for more precise error handling in API operations. The abstract client classes (AbstractAsyncRestClient, AbstractSyncRestClient, AbstractAsyncWebSocketClient, AbstractSyncWebSocketClient) provide a solid foundation for a more organized and reusable client architecture.

These changes are likely to have a positive impact on the overall structure and maintainability of the deepgram-python-sdk. Great work on implementing these improvements!

deepgram/clients/common/v1/helpers.py (2)

1-9: LGTM: File header and imports are correct and well-organized.

The copyright notice, license information, and imports are appropriate for the file's content. The code follows good practices for organizing imports and maintaining readability.

---

1-53: Overall, excellent implementation of helper functions.

This new file successfully introduces reusable utility functions for URL manipulation, specifically for appending query parameters and converting URLs to WebSocket format. These functions will likely contribute to reducing code sprawl within the deepgram-python-sdk, aligning well with the PR objectives.

The implementation is clean, well-documented, and follows Python best practices. With the suggested minor improvements, this file will be a valuable addition to the project.

deepgram/clients/common/__init__.py (3)

5-10: LGTM: Error classes import looks good

The addition of DeepgramApiError and DeepgramUnknownApiError to the import statement is consistent with existing error classes and aligns with the PR objective of enhancing API support. This change improves error handling capabilities for users of the SDK.

---

5-15: Overall, the changes in this file look good and align with the PR objectives

The additions to the __init__.py file enhance the SDK's public API by introducing new error classes and abstract client classes for both REST and WebSocket interactions. These changes support the goal of improving Agent API support in a more organized and reusable manner.

A few minor suggestions were made to improve import style consistency. Additionally, a verification step was requested to ensure the new WebSocket classes are being utilized in the examples as mentioned in the PR description.

Great job on enhancing the SDK's capabilities while maintaining a clean and organized structure!

---

14-15: LGTM: Abstract WebSocket client imports look good

The addition of AbstractAsyncWebSocketClient and AbstractSyncWebSocketClient aligns with the PR objective of organizing the Agent API support in a more reusable manner, specifically for WebSocket functionality. These abstract classes will provide a solid foundation for both asynchronous and synchronous WebSocket interactions.

Consider combining these imports with the previous import statement for consistency:

from .v1 import (
    DeepgramError,
    DeepgramTypeError,
    DeepgramApiError,
    DeepgramUnknownApiError,
    AbstractAsyncRestClient,
    AbstractSyncRestClient,
    AbstractAsyncWebSocketClient,
    AbstractSyncWebSocketClient,
)

The PR mentions that all Speech-to-Text and Text-to-Speech examples using WebSocket functionality were tested. Let's verify the usage of these new WebSocket classes:

deepgram/clients/selfhosted/v1/async_client.py (2)

Line range hint 15-165: Confirmed: Rest of the file unaffected by import change.

I've reviewed the entire file, and I can confirm that the change in the import statement doesn't affect any other part of the file. The AsyncSelfHostedClient class still correctly inherits from AbstractAsyncRestClient, and all methods and their implementations remain unchanged.

---

12-12: LGTM! Verify the new import structure.

The change from a specific import to a common module import looks good. This could be part of a refactoring effort to centralize common classes, which can improve code organization and reusability.

To ensure this change doesn't introduce any issues, please run the following script to verify the new import structure:

These tests will help ensure that:

1. AbstractAsyncRestClient is properly defined in the common module.
2. There are no remaining imports from the old location.
3. There are no potential circular imports introduced by this change.

deepgram/clients/speak/v1/rest/client.py (1)

16-17: Improved import organization

The simplification of import statements enhances code organization and readability. This change aligns well with the PR objective of improving support for the Agent API in a more organized manner.

deepgram/audio/speaker/speaker.py (5)

12-13: LGTM: WebSocket import added

The addition of the websockets import aligns with the PR objectives to enhance support for the Agent API using a shared WebSocket class implementation.

---

282-301: LGTM: Improved thread management and resource cleanup

The changes in the finish method enhance the stability of the thread management logic:

1. Checking for the existence of _thread and _receiver_thread before joining them prevents potential runtime errors.
2. Setting _stream to None after closing it ensures the attribute is in a known state.

These improvements make the cleanup process more robust and less prone to errors.

---

Line range hint 1-324: LGTM: Well-structured and organized implementation

The changes made to this file are targeted and improve specific areas without disrupting the overall organization of the code. The enhancements align well with the PR objectives to support the Agent API in a more organized and reusable manner. The Speaker class remains cohesive and follows a logical flow from initialization to audio playback and cleanup.

---

Line range hint 1-324: Overall assessment: Solid improvements with minor suggestions

The changes made to the Speaker class in this file significantly enhance its robustness and type safety. The improvements in WebSocket exception handling, thread management, and resource cleanup align well with the PR objectives to support the Agent API in a more organized and reusable manner.

Key improvements:

1. Enhanced type safety for class attributes
2. Improved WebSocket exception handling
3. More robust thread management and resource cleanup

Suggestions for further improvement:

1. Add type hints to remaining class attributes for consistency
2. Replace broad exception handling with more specific exceptions where possible
3. Implement additional security measures for WebSocket connections

Overall, these changes represent a solid step forward in the development of the deepgram-python-sdk.

---

Line range hint 1-324: Consider additional security measures for WebSocket connections

While the current implementation looks solid, it's important to ensure that the WebSocket connections are properly secured, especially when dealing with audio data. Consider the following security measures:

1. Implement proper authentication and authorization for WebSocket connections.
2. Use secure WebSocket connections (wss://) in production environments.
3. Validate and sanitize any data received through the WebSocket before processing it.
4. Implement rate limiting to prevent potential DoS attacks.

To verify the security of the WebSocket implementation, you can run the following script:

This script will help identify if secure WebSocket protocols are being used and if there's any authentication mechanism in place for WebSocket connections.

deepgram/clients/common/v1/abstract_sync_rest.py (2)

13-13: LGTM: Improved import statement organization.

The consolidation of error class imports into a single line improves code readability and reduces code sprawl. This change aligns well with the PR objectives.

---

12-13: Verify changes against PR objectives.

The modifications to the import statements align with the PR objectives of enhancing support for the Agent API in a more organized manner and reducing code sprawl. However, it's important to ensure that these changes are part of a broader restructuring effort.

To confirm the extent of the restructuring and its impact on other parts of the codebase, please run the following script:

This script will help us understand if the changes in this file are part of a consistent pattern across the codebase, ensuring that the PR objectives are fully met.

deepgram/clients/analyze/v1/client.py (1)

12-13: Improved import organization

The changes to the import statements simplify the import paths, which aligns well with the PR objective of enhancing support for the Agent API in a more organized manner. This modification improves code maintainability and readability without affecting the functionality.

deepgram/clients/analyze/v1/async_client.py (1)

12-13: LGTM! Verify import paths in the codebase.

The consolidation of imports from the ...common module simplifies the import structure. This change doesn't affect the functionality but may indicate a reorganization of the project's module structure.

To ensure consistency across the codebase, run the following script to check for any remaining occurrences of the old import paths:

✅ Verification successful

Import paths successfully consolidated. No outdated imports found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for old import paths in the codebase

# Search for old AbstractAsyncRestClient import
echo "Checking for old AbstractAsyncRestClient import:"
rg --type python "from \.\.\.abstract_async_client import AbstractAsyncRestClient"

# Search for old DeepgramError and DeepgramTypeError imports
echo "Checking for old DeepgramError and DeepgramTypeError imports:"
rg --type python "from \.\.\.common\.v1\.errors import DeepgramError, DeepgramTypeError"

Length of output: 477

---

Script:

#!/bin/bash
# Description: Check for old import paths in the codebase

# Search for old AbstractAsyncRestClient import
echo "Checking for old AbstractAsyncRestClient import:"
rg -g "*.py" "from \.\.\.abstract_async_client import AbstractAsyncRestClient"

# Search for old DeepgramError and DeepgramTypeError imports
echo "Checking for old DeepgramError and DeepgramTypeError imports:"
rg -g "*.py" "from \.\.\.common\.v1\.errors import DeepgramError, DeepgramTypeError"

Length of output: 407

deepgram/clients/listen/v1/rest/client.py (1)

12-13: LGTM: Import statements consolidated.

The import statements have been simplified by consolidating them under the ....common module. This change improves code organization and maintainability.

Let's verify the usage of these imported classes throughout the file:

✅ Verification successful

Verified: Import statements usage confirmed.

The consolidated imports are correctly utilized throughout the client.py file, ensuring proper functionality and maintainability.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the usage of AbstractSyncRestClient, DeepgramError, and DeepgramTypeError

echo "Checking usage of AbstractSyncRestClient:"
rg "class ListenRESTClient\(AbstractSyncRestClient\)" deepgram/clients/listen/v1/rest/client.py

echo "\nChecking usage of DeepgramError:"
rg "raise DeepgramError" deepgram/clients/listen/v1/rest/client.py

echo "\nChecking usage of DeepgramTypeError:"
rg "raise DeepgramTypeError" deepgram/clients/listen/v1/rest/client.py

Length of output: 1114

deepgram/clients/listen/v1/rest/async_client.py (3)

12-13: Improved import structure enhances code organization.

The changes to the import statements are a positive step towards better code organization. By importing from a common module (....common) instead of using relative imports, this modification:

1. Enhances code reusability across the project.
2. Improves maintainability by centralizing common classes.
3. Aligns with the PR objective of organizing the Agent API support more effectively.

These changes contribute to reducing code sprawl within the deepgram-python-sdk, as mentioned in the PR objectives.

---

12-13: Changes align with PR objectives and suggest broader refactoring.

The modifications to the import statements in this file align well with the PR objectives of enhancing support for the Agent API in a more organized and reusable manner. By moving common classes to a shared module, this change contributes to reducing code sprawl within the deepgram-python-sdk.

To ensure consistency across the project:

1. Check if similar import changes have been applied to other relevant files in the SDK.
2. Verify that the common module (....common) is properly structured and contains all necessary shared classes.

#!/bin/bash
# Check for similar import patterns across the SDK
rg --type python -e "from ....common import AbstractAsyncRestClient, DeepgramError, DeepgramTypeError" deepgram/

# Verify the structure of the common module
ls -R deepgram/clients/common/

---

Line range hint 26-26: Verify unchanged functionality of AsyncListenRESTClient.

The changes to the import statements don't affect the functionality of the AsyncListenRESTClient class. The class still correctly inherits from AbstractAsyncRestClient, and the usage of DeepgramError and DeepgramTypeError remains unchanged throughout the methods.

To ensure that the import changes haven't inadvertently affected any functionality, please run the following verification steps:

1. Execute all existing unit tests related to this file.
2. Manually test the AsyncListenRESTClient class with a sample audio file to confirm that transcription still works as expected.

examples/text-to-speech/websocket/async_complete/main.py (1)

19-20: 🛠️ Refactor suggestion

Avoid using global variables for warning_notice

Using global variables can make the code harder to understand and maintain. Consider encapsulating warning_notice within a class or using closures to maintain state.

⛔ Skipped due to learnings

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#434
File: examples/text-to-speech/websocket/async_complete/main.py:68-79
Timestamp: 2024-08-07T19:24:31.383Z
Learning: When the user indicates that a piece of code is only an example, consider suggesting the inclusion of best practices, such as error handling, to promote good coding habits.

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#434
File: examples/text-to-speech/websocket/async_complete/main.py:21-27
Timestamp: 2024-08-07T19:24:37.020Z
Learning: When the user indicates that a piece of code is only an example, consider suggesting the inclusion of best practices, such as error handling, to promote good coding habits.

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#434
File: examples/text-to-speech/websocket/async_complete/main.py:32-58
Timestamp: 2024-08-07T19:24:27.278Z
Learning: When the user indicates that a piece of code is only an example, acknowledge their comment and note the context for future reviews without insisting on changes unless they significantly enhance the clarity or functionality of the example.

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#434
File: examples/text-to-speech/websocket/async_complete/main.py:90-93
Timestamp: 2024-08-07T19:24:46.887Z
Learning: When the user indicates that a piece of code is only an example, acknowledge their comment and note the context for future reviews without insisting on changes unless they significantly enhance the clarity or functionality of the example.

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#434
File: examples/text-to-speech/websocket/async_interactive/main.py:0-0
Timestamp: 2024-08-06T20:02:34.130Z
Learning: When the user indicates that a piece of code is only an example, consider suggesting the inclusion of best practices, such as error handling, to promote good coding habits.

deepgram/clients/listen/v1/websocket/client.py (3)

35-37: Inheritance from AbstractSyncWebSocketClient

The ListenWebSocketClient class now inherits from AbstractSyncWebSocketClient, promoting code reuse and reducing redundancy. This change streamlines the client implementation.

---

Line range hint 525-542: Ensure all threads are properly managed upon finishing

The finish method attempts to join several threads. Ensure that all threads started by the client are accounted for and properly terminated to prevent resource leaks.

Review the thread management logic to confirm that:

• Threads are started with appropriate names for identification.
• All threads are joined or terminated when the client finishes.
• There are no potential race conditions or deadlocks during thread shutdown.

---

240-325: ⚠️ Potential issue

Verify compatibility of match statements with Python version

The match statement used for pattern matching requires Python 3.10 or newer. Ensure that the project's minimum Python version supports this feature to avoid runtime errors.

Run the following script to check the required Python version:

If backward compatibility is needed, consider refactoring the match statement to use if-elif-else constructs.

deepgram/clients/listen/v1/websocket/async_client.py (4)

35-37: Enhance Code Reusability by Extending AbstractAsyncWebSocketClient

The AsyncListenWebSocketClient class now extends AbstractAsyncWebSocketClient, promoting code reuse and reducing redundancy by leveraging common WebSocket client functionality.

---

85-86: Properly Initialize the Base Class

The constructor now calls super().__init__(self._config, self._endpoint), ensuring that the parent class is correctly initialized. This is essential for the proper functioning of inherited methods and attributes.

---

134-146: Delegate Connection Logic to Parent Class in start Method

By calling super().start(...) in the start method, the connection setup and error handling are centralized in the parent class, enhancing maintainability and reducing code duplication.

---

510-511: Ensure Availability of send Method

The _close_message method calls await self.send(...), but the send method has been removed from this class. Ensure that the send method is available in the parent class AbstractAsyncWebSocketClient.

Run the following script to verify that the send method is implemented in the class hierarchy:

deepgram/clients/speak/v1/websocket/client.py (10)

8-8: Import necessary typing objects

The addition of Callable to the imports is appropriate, ensuring that callable types can be correctly annotated throughout the code.

---

15-16: Inherit from AbstractSyncWebSocketClient and import DeepgramError

The import of AbstractSyncWebSocketClient and DeepgramError sets up the foundation for proper inheritance and custom exception handling.

---

38-40: Updated class inheritance for code reuse

The SpeakWSClient class now inherits from AbstractSyncWebSocketClient, promoting code reuse and consistency across WebSocket clients.

---

69-71: Validate the config parameter in the constructor

The check for config is None and raising a DeepgramError ensures that the class is initialized with the necessary configuration, preventing potential issues later on.

---

119-121: Call parent constructor with appropriate arguments

Calling super().__init__(self._config, self._endpoint) ensures that the base class is properly initialized, maintaining the integrity of the inheritance structure.

---

168-171: Delegate listening to the Speaker instance if available

By delegating listening to self._speaker, the code efficiently manages audio playback without the need for a separate listening thread.

---

Line range hint 255-384: Using match statements improves message processing

The implementation of match statements in _process_text provides clear and concise handling of different message types, leveraging Python 3.10 features.

---

386-398: Efficient handling of binary messages in _process_binary

The _process_binary method correctly processes binary data and emits the AudioData event, ensuring binary messages are handled appropriately.

---

Line range hint 600-630: Ensure graceful shutdown in finish method

Properly stopping threads and cleaning up resources in the finish method prevents resource leaks and ensures the client shuts down gracefully.

---

649-649: Provide class alias for backward compatibility

Defining SpeakWebSocketClient as an alias for SpeakWSClient maintains backward compatibility and eases the transition for existing codebases.

deepgram/clients/speak/v1/websocket/async_client.py (10)

8-8: Appropriate addition of type hints

The added imports from the typing module enhance code readability and maintainability by providing explicit type hints.

---

15-16: Necessary imports for subclassing and error handling

Importing AbstractAsyncWebSocketClient and DeepgramError is appropriate for extending the base functionality and consistent error management.

---

38-40: Refactoring class to inherit from AbstractAsyncWebSocketClient

Refactoring AsyncSpeakWSClient to inherit from AbstractAsyncWebSocketClient centralizes WebSocket functionalities, promoting code reuse and maintainability.

---

68-69: Improved initialization with config validation

Adding a check for a None configuration and raising a DeepgramError enhances robustness by ensuring that the client is initialized with valid settings.

---

97-101: Detailed logging of speaker settings

Logging the rate, channels, and device_index provides valuable debugging information, aiding in the troubleshooting of speaker initialization issues.

---

116-118: Correct invocation of the parent class constructor

Calling super().__init__(self._config, self._endpoint) ensures that the base class is properly initialized, maintaining the integrity of the class hierarchy.

---

165-169: Delegating listening to the speaker instance

Utilizing super().delegate_listening(self._speaker) correctly delegates the listening responsibilities to the speaker, aligning with the refactored architecture.

---

Line range hint 258-359: Verify compatibility of match statements with Python versions

The use of match statements requires Python 3.10 or newer. Ensure that the codebase and deployment environments support Python 3.10+. If support for older versions is needed, consider replacing match with if-elif constructs.

---

Line range hint 605-646: Proper exception handling in finish method

The finish method appropriately handles asyncio.CancelledError and asyncio.TimeoutError, ensuring graceful shutdown and resource cleanup.

---

668-668: Creation of class alias for backward compatibility

Creating AsyncSpeakWebSocketClient as an alias to AsyncSpeakWSClient maintains backward compatibility and eases the transition for existing codebases.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)

deepgram/clients/listen/v1/websocket/client.py (2)

Line range hint 461-461: Remove call to non-existent method _signal_exit

The method _signal_exit() is called, but it has been removed from the class. This will result in an AttributeError at runtime.

Consider updating the code to properly handle exit without calling _signal_exit(). If necessary, implement the exit logic within _flush or recreate the _signal_exit method.

---

Line range hint 1-576: Overall improvements with minor issues to address

The refactoring of ListenWebSocketClient to inherit from AbstractSyncWebSocketClient is a significant improvement that should reduce code duplication and improve maintainability. The changes to methods like start, finish, and the addition of _process_text and _close_message are well-implemented.

However, there are a few issues to address:

1. Correct the class name in log messages (lines 145-146).
2. Handle JSON decoding errors more specifically in _process_text.
3. Either implement or remove the _process_binary method.
4. Remove or replace calls to the non-existent _signal_exit method in error handling blocks.

After addressing these issues, the code will be more robust and consistent.

deepgram/clients/speak/v1/websocket/client.py (3)

Line range hint 255-373: Improved message processing with better structure

The _process_text method has been significantly improved:

1. Separation of text and binary processing enhances clarity and maintainability.
2. Use of match-case statement improves readability for different response types.
3. Improved logging provides better diagnostics.

However, there's still room for improvement in exception handling:

Consider catching more specific exceptions instead of using a broad except Exception. This will make it easier to debug and handle different error scenarios appropriately. For example:

try:
    # ... existing code ...
except json.JSONDecodeError as e:
    self._logger.error("Invalid JSON in message: %s", e)
    # Handle JSON parsing error
except KeyError as e:
    self._logger.error("Missing expected key in message: %s", e)
    # Handle missing key error
except Exception as e:
    self._logger.error("Unexpected error in _process_text: %s", e)
    # Handle other unexpected errors

This approach will provide more specific error handling and logging.

---

Line range hint 399-457: Enhanced flush method with improved error handling

The _flush method has been improved with better error handling and the use of _exit_event for graceful termination. These changes enhance the robustness of the method.

However, similar to the _process_text method, there's room for improvement in exception handling:

Consider catching more specific exceptions instead of using a broad except Exception. This will provide better error diagnostics and handling. For example:

try:
    # ... existing code ...
except ValueError as e:
    self._logger.error("Invalid value in _flush: %s", e)
    # Handle value error
except RuntimeError as e:
    self._logger.error("Runtime error in _flush: %s", e)
    # Handle runtime error
except Exception as e:
    self._logger.error("Unexpected error in _flush: %s", e)
    # Handle other unexpected errors

This approach will allow for more targeted error handling and logging.

---

473-481: Method signature aligned with abstract base class

The send method signature has been updated to accept Union[str, bytes], aligning with the abstract base class. This change addresses the learning from previous reviews about maintaining consistency with abstract base classes to satisfy linters like pylint.

However, the implementation doesn't actually handle bytes data. To make this more explicit:

Consider updating the type hint to be more specific about what the method actually handles:

def send(self, data: str) -> bool:
    """
    Sends text data. Does not support bytes data.
    """
    return self.send_text(data)

This makes it clear that the method only supports string input, despite the more general signature required by the abstract base class.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b2b07b9 and f5f24b7.

📒 Files selected for processing (30)

• deepgram/audio/speaker/speaker.py (4 hunks)
• deepgram/clients/init.py (1 hunks)
• deepgram/clients/analyze/v1/async_client.py (1 hunks)
• deepgram/clients/analyze/v1/client.py (1 hunks)
• deepgram/clients/common/init.py (1 hunks)
• deepgram/clients/common/v1/init.py (1 hunks)
• deepgram/clients/common/v1/abstract_async_rest.py (1 hunks)
• deepgram/clients/common/v1/abstract_async_websocket.py (1 hunks)
• deepgram/clients/common/v1/abstract_sync_rest.py (1 hunks)
• deepgram/clients/common/v1/abstract_sync_websocket.py (1 hunks)
• deepgram/clients/common/v1/errors.py (1 hunks)
• deepgram/clients/common/v1/helpers.py (1 hunks)
• deepgram/clients/common/v1/websocket_events.py (1 hunks)
• deepgram/clients/errors.py (0 hunks)
• deepgram/clients/helpers.py (0 hunks)
• deepgram/clients/listen/v1/rest/async_client.py (1 hunks)
• deepgram/clients/listen/v1/rest/client.py (1 hunks)
• deepgram/clients/listen/v1/websocket/async_client.py (11 hunks)
• deepgram/clients/listen/v1/websocket/client.py (11 hunks)
• deepgram/clients/listen/v1/websocket/response.py (0 hunks)
• deepgram/clients/manage/v1/async_client.py (1 hunks)
• deepgram/clients/manage/v1/client.py (1 hunks)
• deepgram/clients/selfhosted/v1/async_client.py (1 hunks)
• deepgram/clients/selfhosted/v1/client.py (1 hunks)
• deepgram/clients/speak/v1/rest/async_client.py (1 hunks)
• deepgram/clients/speak/v1/rest/client.py (1 hunks)
• deepgram/clients/speak/v1/websocket/async_client.py (21 hunks)
• deepgram/clients/speak/v1/websocket/client.py (18 hunks)
• examples/text-to-speech/websocket/async_complete/main.py (4 hunks)
• examples/text-to-speech/websocket/complete/main.py (2 hunks)

💤 Files not reviewed due to no reviewable changes (3)

• deepgram/clients/errors.py
• deepgram/clients/helpers.py
• deepgram/clients/listen/v1/websocket/response.py

🚧 Files skipped from review as they are similar to previous changes (25)

• deepgram/audio/speaker/speaker.py
• deepgram/clients/init.py
• deepgram/clients/analyze/v1/async_client.py
• deepgram/clients/analyze/v1/client.py
• deepgram/clients/common/init.py
• deepgram/clients/common/v1/init.py
• deepgram/clients/common/v1/abstract_async_rest.py
• deepgram/clients/common/v1/abstract_async_websocket.py
• deepgram/clients/common/v1/abstract_sync_rest.py
• deepgram/clients/common/v1/abstract_sync_websocket.py
• deepgram/clients/common/v1/errors.py
• deepgram/clients/common/v1/helpers.py
• deepgram/clients/common/v1/websocket_events.py
• deepgram/clients/listen/v1/rest/async_client.py
• deepgram/clients/listen/v1/rest/client.py
• deepgram/clients/listen/v1/websocket/async_client.py
• deepgram/clients/manage/v1/async_client.py
• deepgram/clients/manage/v1/client.py
• deepgram/clients/selfhosted/v1/async_client.py
• deepgram/clients/selfhosted/v1/client.py
• deepgram/clients/speak/v1/rest/async_client.py
• deepgram/clients/speak/v1/rest/client.py
• deepgram/clients/speak/v1/websocket/async_client.py
• examples/text-to-speech/websocket/async_complete/main.py
• examples/text-to-speech/websocket/complete/main.py

🧰 Additional context used

📓 Learnings (1)

deepgram/clients/speak/v1/websocket/client.py (2)

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#468
File: deepgram/clients/speak/v1/websocket/client.py:477-481
Timestamp: 2024-09-27T15:17:25.060Z
Learning: When maintaining consistency with abstract base classes and satisfying linters like pylint, it's acceptable to keep method signatures like `send(self, data: Union[str, bytes])` even if the method doesn't handle `bytes`.

Learnt from: dvonthenen
PR: deepgram/deepgram-python-sdk#468
File: deepgram/clients/speak/v1/websocket/async_client.py:475-483
Timestamp: 2024-09-27T15:17:06.865Z
Learning: When overriding abstract methods like `send` that accept `Union[bytes, str]`, even if the subclass only supports `str`, the method signature should remain the same to satisfy linters like pylint and maintain consistency with the abstract base class.

🔇 Additional comments (14)

deepgram/clients/listen/v1/websocket/client.py (7)

7-7: Improved class structure and inheritance

The ListenWebSocketClient now inherits from AbstractSyncWebSocketClient, which is a good refactoring decision. This change likely reduces code duplication and improves maintainability by centralizing common WebSocket functionality in the parent class.

Also applies to: 14-15, 35-37

---

509-511: New method for closing the stream

The _close_message method is a good addition for explicitly closing the stream. It sends a properly formatted JSON message to indicate the stream should be closed.

---

519-522: Improved finish method with parent class call

The finish method now correctly calls the parent class's finish method before performing its own cleanup. This ensures that any necessary actions in the parent class are executed. The error handling for the parent method call is also appropriate.

---

224-354: ⚠️ Potential issue

Handle potential JSON decoding errors

When parsing the incoming message with json.loads(message), there's a risk of a json.JSONDecodeError if the message is not valid JSON. Currently, this exception is caught by the general except Exception block, but it's better practice to catch specific exceptions.

Consider adding a specific exception handler for JSON decoding:

try:
    data = json.loads(message)
except json.JSONDecodeError as e:
    self._logger.error("Failed to decode JSON message: %s", e)
    # Optionally emit an error event or handle the invalid message
    return

---

358-360: ⚠️ Potential issue

Implement _process_binary or remove it

The _process_binary method raises NotImplementedError, indicating that binary messages are not expected. If binary data will not be processed, consider removing this method. Otherwise, implement the necessary logic to handle binary messages properly.

Decide whether to support binary messages:

• If supporting, implement the method.
• If not, and if the superclass does not require it, safely remove the method.

---

398-398: ⚠️ Potential issue

Remove call to non-existent method _signal_exit

The method _signal_exit() is called, but it has been removed from the class. This will result in an AttributeError at runtime.

Consider updating the code to properly handle exit without calling _signal_exit(). If necessary, implement the exit logic within _keep_alive or recreate the _signal_exit method.

---

135-147: ⚠️ Potential issue

Correct the class name in log messages

The log messages reference AsyncListenWebSocketClient, but the class is ListenWebSocketClient. This could cause confusion during debugging.

Apply this diff to correct the class name:

-                self._logger.error("AsyncListenWebSocketClient.start failed")
-                self._logger.debug("AsyncListenWebSocketClient.start LEAVE")
+                self._logger.error("ListenWebSocketClient.start failed")
+                self._logger.debug("ListenWebSocketClient.start LEAVE")

Likely invalid or redundant comment.

deepgram/clients/speak/v1/websocket/client.py (7)

8-8: Improved class structure through inheritance

The changes to the import statements and class definition enhance the overall structure of the SpeakWSClient. By inheriting from AbstractSyncWebSocketClient, the class can leverage common functionality and maintain a consistent interface. This is a good practice for code reuse and maintainability.

Also applies to: 15-16, 38-40

---

69-71: Improved initialization and error handling

The changes to the __init__ method enhance error handling and leverage the parent class's functionality. The addition of the config check prevents potential issues, and the flexible _speaker initialization allows for better customization.

Minor suggestion: Consider making the error message for the DeepgramError more specific, e.g., "Config cannot be None".

Also applies to: 101-104, 119-120

---

168-189: Improved start method with better error handling

The changes to the start method significantly improve its structure and functionality:

1. Delegation to the speaker (if present) enhances separation of concerns.
2. Leveraging the parent class's start method reduces code duplication.
3. Improved error handling and logging provide better diagnostics.

These changes make the method more robust and easier to maintain.

Also applies to: 214-219

---

386-397: Good addition of binary data processing

The new _process_binary method is a good addition that improves the separation of concerns between text and binary data processing. It's implemented in a simple and straightforward manner, emitting an AudioData event with the received binary data.

---

537-550: Improved send_raw method with better error handling

The changes to the send_raw method enhance its robustness:

1. Improved error handling with specific exception catching.
2. Better logging for debugging purposes.
3. Utilization of the parent class's send method, which is a good practice for code reuse.

These improvements make the method more reliable and easier to debug.

---

Line range hint 600-630: Enhanced finish method with improved cleanup

The changes to the finish method significantly improve the cleanup process:

1. Utilization of the parent class's finish method, which is a good practice for maintaining the inheritance chain.
2. Improved thread management, ensuring that all created threads (speaker and flush) are properly stopped and joined.
3. Better logging for debugging purposes.

These improvements ensure a more thorough and reliable cleanup process when closing the WebSocket connection.

---

649-649: Good addition of backwards-compatible alias

The addition of SpeakWebSocketClient = SpeakWSClient at the end of the file is a good practice. It provides backwards compatibility if the class was previously referred to as SpeakWebSocketClient in other parts of the codebase or by users of the library. This helps maintain API stability while allowing for the new, more concise class name.