Expose All Response Types/Objects #464
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 2 minutes and 13 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. WalkthroughThe changes involve a comprehensive restructuring of import statements and organization of classes across multiple files in the Deepgram SDK. New entities have been added, existing entities have been reorganized into categorized sections, and several classes have been renamed or removed to streamline the codebase. The modifications focus on improving clarity and maintainability without altering the core functionality of the SDK. 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 (
|
davidvonthenen
force-pushed
the
issue457-expose-all-response-types
branch
2 times, most recently
from
ab2aea8 to
b2c6dce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 27
Outside diff range and nitpick comments (46)
deepgram/clients/live/v1/__init__.py (1)
13-14: LGTM! Consider updating documentation.The addition of
ListenWSMetadataResponseand retainingMetadataResponsefor backward compatibility aligns well with the PR objectives. This change successfully exposes the new type at the top level while maintaining support for existing code.Consider updating the documentation to:
- Explain the difference between
ListenWSMetadataResponseandMetadataResponse.- Guide users on when to use each type.
- Indicate that
MetadataResponseis maintained for backward compatibility but may be deprecated in future versions.This will help users understand and adopt the new structure more effectively.
deepgram/clients/listen/v1/websocket/__init__.py (1)
22-30: Great further categorization of response classesThe additional categorization of response classes into "between rest and websocket" and "unique" sections further enhances the clarity and organization of the imports. This change aligns perfectly with the PR objective of exposing all types and classes at the top level, making it easier for users to access any object in a response by its name or reference.
One minor suggestion for consistency:
Consider adding a comment for the "Metadata" class similar to the other categories:
Channel, Word, - #### unique - Metadata, + #### unique + Metadata,This small change would maintain the consistent style throughout the import section.
deepgram/clients/analyze/v1/__init__.py (2)
17-24: LGTM! Analyze-specific options grouped together.The reorganization of imports improves code structure by grouping analyze-specific options together. This change, along with the addition of
AnalyzeStreamSourceandAnalyzeSource, supports the PR objective of exposing all types at the top level.Consider adding a blank line before the "analyze" comment (line 17) for consistency with the "common" section above.
42-46: LGTM! Unique response types added to complete the set.The addition of
Metadata,Results, andSummaryas unique response types completes the set of exposed types, fully aligning with the PR objective. This change enhances the SDK's completeness and usability without breaking existing functionality.Consider adding a blank line before the "unique" comment (line 42) for consistency with the "shared" section above and to improve readability.
deepgram/clients/common/v1/__init__.py (1)
Line range hint
1-47: Overall, the changes successfully implement the PR objectives with a minor concern.The modifications to this
__init__.pyfile effectively expose all types and classes at the top level of the response results, aligning with the PR objectives. The import structure is clean, logical, and follows Python best practices.However, there's one minor concern to address:
- Potential naming conflict: The
Sentimentclass is imported from bothenumsandrest_responsemodules. To resolve this, consider one of the following options:
a. Use an alias for one of the imports to differentiate them.
b. If they represent the same concept, consider consolidating them into a single import.
c. If they are different and both necessary, consider renaming one to avoid confusion.To resolve the naming conflict, you could modify the import statement as follows:
from .enums import Sentiment as EnumSentiment # ... other imports ... from .rest_response import ( # ... other imports ... Sentiment as ResponseSentiment, # ... other imports ... )This approach clearly distinguishes between the two
Sentimentclasses while maintaining backward compatibility.deepgram/clients/speak/v1/__init__.py (2)
5-19: Improved import organization and exposure of types.The reorganization of imports from the
.restmodule enhances code readability and aligns with the PR objective of exposing all types at the top level. The categorization into "top level," "common," and "unique" sections provides a clear structure.There's a minor inconsistency in naming convention:
- Line 18 uses
SpeakRESTSource- Line 17 uses
SpeakRestSourceConsider standardizing to either
RESTorRestfor consistency.
28-47: Improved websocket import organization and type exposure.The reorganization of imports from the
.websocketmodule enhances code readability and aligns with the PR objective of exposing all types at the top level. The categorization into "top level" and "shared" sections provides a clear structure.The alias
SpeakWSMetadataResponseforMetadataResponseis a good practice to avoid potential naming conflicts.Consider grouping all imports from
.websocketinto a singlefrom .websocket import (...)statement for better consistency with the rest imports. This would make the import structure more uniform across the file.deepgram/clients/manage/__init__.py (1)
33-52: LGTM: Comprehensive additions enhance SDK functionalityThe extensive additions to the "shared" section significantly enhance the SDK's functionality and align well with the PR objectives. The inclusion of various types like
STTDetails,TTSMetadata,Member, etc., improves the SDK's flexibility and usability. MovingBalanceto this section is logical and maintains consistency.These changes appear to be backwards compatible and greatly improve the SDK's capabilities.
Consider grouping related types together (e.g., STT-related, TTS-related, Usage-related) for improved readability. For example:
# STT-related STTDetails, STTUsageDetails, STTTokens, # TTS-related TTSMetadata, TTSDetails, TTSUsageDetails, TTSTokens, # Usage-related UsageSummaryResults, UsageModel, # Other types...deepgram/clients/speak/__init__.py (1)
14-25: LGTM: Well-organized imports with a suggestionThe categorization of imports into "top level", "common", and "unique" sections greatly improves code organization and readability. Moving SpeakOptions to the top level aligns with the PR objective of exposing all types at the top level. The addition of various Source types provides more granular control over input sources, which is beneficial for users.
Consider adding a brief comment above each category (top level, common, unique) to explain their purpose or criteria for inclusion. This would further enhance code readability and maintainability.
deepgram/clients/listen/v1/rest/__init__.py (1)
46-58: Excellent addition of unique response types with a suggestion for documentation.The inclusion of numerous unique response types (e.g.,
Entity,Metadata,Paragraph,Results) greatly enhances the SDK's capability to handle diverse response data. This aligns perfectly with the PR objective of exposing all response types at the top level.Consider adding a brief inline comment or docstring to explain the distinction between "shared", "unique", and "between rest and websocket" categories. This would further improve the code's self-documentation and help users understand the purpose and scope of each category.
deepgram/clients/common/v1/websocket_response.py (4)
16-22: Consider using a more specific type for the 'type' attribute.The
OpenResponseclass looks good overall, but consider the following suggestion:Instead of initializing
typeas an empty string, it might be more appropriate to use a more specific type or a default value that accurately represents an "open" message. This could improve type safety and make the code more self-documenting.Consider modifying the class definition as follows:
from typing import Literal @dataclass class OpenResponse(BaseResponse): """ Open Message from the Deepgram Platform """ type: Literal["open"] = "open"This change ensures that
typecan only be "open" and provides a meaningful default value.
28-34: Apply the same type improvement to CloseResponse.Similar to the
OpenResponseclass, consider improving the type definition for theCloseResponseclass:Modify the class definition as follows:
from typing import Literal @dataclass class CloseResponse(BaseResponse): """ Close Message from the Deepgram Platform """ type: Literal["close"] = "close"This change ensures type safety and provides a meaningful default value for the
typeattribute.
40-51: Good use of Optional and custom metadata, consider improving attribute initialization.The
ErrorResponseclass is well-structured with appropriate use ofOptionaland custom metadata for JSON serialization. However, consider the following suggestions:
Instead of initializing
description,message, andtypeas empty strings, consider usingNoneas the default value. This makes it clear when these fields haven't been set.Use
field(default=None)for these attributes to maintain consistency with thevariantattribute.Here's a suggested improvement:
@dataclass class ErrorResponse(BaseResponse): """ Error Message from the Deepgram Platform """ description: Optional[str] = field(default=None) message: Optional[str] = field(default=None) type: Optional[str] = field(default=None) variant: Optional[str] = field( default=None, metadata=dataclass_config(exclude=lambda f: f is None) )This change improves consistency and makes it clearer when fields haven't been set.
57-64: Consider improving attribute initialization for UnhandledResponse.The
UnhandledResponseclass structure is clear, but the attribute initialization can be improved:
- For the
typeattribute, consider using a more specific type or a default value that accurately represents an "unhandled" message.- For the
rawattribute, consider usingNoneas the default value to clearly indicate when it hasn't been set.Here's a suggested improvement:
from typing import Literal, Optional @dataclass class UnhandledResponse(BaseResponse): """ Unhandled Message from the Deepgram Platform """ type: Literal["unhandled"] = "unhandled" raw: Optional[str] = field(default=None)This change improves type safety for the
typeattribute and makes it clearer when therawfield hasn't been set.deepgram/clients/live/v1/client.py (1)
11-11: Approve renaming of MetadataResponse importThe renaming of
MetadataResponsetoListenWSMetadataResponseimproves clarity by specifying its relation to WebSocket listening. This change aligns well with the PR objective of exposing all types at the top level.Consider adding a brief comment explaining the purpose of this specific response type to enhance code readability. For example:
# Response type for WebSocket listening metadata ListenWSMetadataResponse as ListenWSMetadataResponseLatest,deepgram/clients/speak/v1/rest/options.py (1)
61-64: Approve type aliases, improve comment, and address pylint issue.The new type aliases
SpeakSource,SpeakRestSource, andSpeakRESTSourceare good additions, providing clear definitions for the types of sources accepted by the Speak API.However, there are a few points to consider:
The comment "unique" is vague. Consider replacing it with a more descriptive comment explaining the purpose of these aliases.
The pylint disable comment for
SpeakRESTSourcesuggests a naming convention issue. Instead of disabling the pylint check, consider renaming the alias to follow the naming convention (e.g.,SpeakRestSource).
SpeakRestSourceandSpeakRESTSourceare identical. Consider keeping only one of them to avoid confusion.Here's a suggested improvement:
# Define source types for the Speak API SpeakSource = Union[FileSource, BufferedReader] SpeakRestSource = SpeakSourcedeepgram/clients/analyze/v1/options.py (1)
83-83: Clarify the relationship between AnalyzeSource and AnalyzeStreamSource.While
AnalyzeStreamSourceis still defined, it's no longer part ofAnalyzeSource. This separation might not be immediately clear to users.Consider adding a comment explaining the relationship between these types and why
AnalyzeStreamSourceis separate fromAnalyzeSource. Additionally, update the documentation to reflect this change in the type hierarchy.deepgram/clients/analyze/client.py (1)
Line range hint
80-87: LGTM: Client aliases are correctly defined. Consider adding a clarification comment.The aliases for AnalyzeClient and AsyncAnalyzeClient are properly defined, pointing to their respective "Latest" versions. The additional aliases (ReadClient and AsyncReadClient) are also correctly set up.
Consider adding a brief comment explaining the purpose of the ReadClient and AsyncReadClient aliases. For example:
# Aliases for backwards compatibility or alternative naming ReadClient = AnalyzeClientLatest AsyncReadClient = AsyncAnalyzeClientLatestThis would provide clarity on why these additional aliases exist.
deepgram/clients/speak/client.py (3)
Line range hint
5-50: Improved import organization and versioning.The reorganization of imports and the addition of "Latest" suffixes to class aliases significantly enhance code readability and maintainability. This structure clearly indicates the most up-to-date versions of classes and provides a good overview of the SDK's components.
Consider adding comments to separate the import groups more clearly, for example:
# REST-related imports from .v1 import ( ... ) # WebSocket-related imports from .v1 import ( ... ) # Response type imports from .v1 import ( ... )This would further improve readability, especially as the number of imports grows.
Line range hint
55-83: Well-structured alias declarations for backward compatibility.The alias declarations are well-organized, maintaining backward compatibility while allowing for future version updates. The grouping into REST and WebSocket sections, further divided into input and output classes, significantly improves code readability and organization.
Regarding the
# pylint: disable=invalid-namecomment on line 65:SpeakRESTSource = SpeakRESTSourceLatest # pylint: disable=invalid-nameConsider moving this pylint disable to the top of the file or creating a
.pylintrcfile to globally configure this rule if it applies to multiple lines. This would reduce inline comments and improve overall code cleanliness.
Line range hint
86-97: Comprehensive client declarations with backward compatibility.The declarations effectively maintain backward compatibility while providing clear entry points for both REST and WebSocket clients, including their asynchronous counterparts. This approach ensures flexibility for various use cases and smooth transitions for existing users.
To improve clarity, consider adding a brief comment explaining the purpose of the backward compatibility aliases. For example:
# Backward compatibility aliases for users of older SDK versions SpeakResponse = SpeakRESTResponseLatest SpeakClient = SpeakRESTClientLatest # Current client classes SpeakRESTClient = SpeakRESTClientLatest AsyncSpeakRESTClient = AsyncSpeakRESTClientLatest # ... (rest of the client declarations)This would help future maintainers understand the rationale behind these aliases.
deepgram/clients/manage/client.py (3)
31-54: LGTM: Comprehensive addition of new response classesThe extensive addition of new response classes significantly enhances the SDK's capability to handle various response types, aligning well with the PR objectives. The naming conventions are consistent, and the imports are logically organized.
Consider grouping the imports more explicitly, perhaps with comments separating different categories (e.g., "General", "STT-related", "TTS-related", "Usage-related"). This could improve readability and make it easier to locate specific imports in the future.
83-106: LGTM: Comprehensive addition of new response class assignmentsThe extensive addition of assignments for the newly imported response classes is consistent with the PR objectives and maintains the pattern of aliasing the latest version of each class. This ensures that all new types are exposed at the top level while maintaining backwards compatibility.
To improve readability and maintain consistency with the import section, consider grouping these assignments more explicitly, perhaps with comments separating different categories (e.g., "General", "STT-related", "TTS-related", "Usage-related"). This would make it easier to locate specific assignments and maintain parallelism with the import section.
Line range hint
1-106: Overall LGTM: Successful exposure of all response types/objectsThe changes in this file successfully achieve the PR objective of exposing all types and classes at the top level of the response results. The modifications:
- Add numerous new response classes to the imports.
- Create corresponding assignments for these new classes.
- Maintain backwards compatibility by following the existing pattern of aliasing the latest versions.
- Preserve consistent naming conventions and code structure.
These changes enhance the SDK's capability to handle various response types while ensuring that existing client code remains unaffected. The expanded set of response classes, particularly in STT and TTS functionalities, provides users with more granular access to response data.
As the number of response types grows, consider implementing a more scalable approach to organizing and documenting these types. This could involve:
- Creating separate modules for different categories of response types (e.g.,
stt_responses.py,tts_responses.py,usage_responses.py).- Implementing a dynamic import mechanism to reduce the number of explicit imports in this file.
- Generating API documentation that clearly outlines all available response types and their structures.
These steps would improve maintainability and make it easier for users to understand and utilize the full range of response types available in the SDK.
deepgram/clients/listen/v1/websocket/options.py (2)
Line range hint
48-57: Address the TODO comment for theendpointingfield type.The TODO comment indicates that the
endpointingfield type needs to be updated in a future release. To address this:
- Create a separate issue to track this change and link it to this PR.
- Plan for a deprecation cycle:
a. In the next minor release, add a deprecation warning when a string value is used.
b. In a future major release, change the type toOptional[Union[bool, int]].- Update the documentation to reflect the planned change and guide users on migrating their code.
Would you like me to create a GitHub issue to track this TODO and outline the deprecation plan?
Line range hint
17-157: Consider refactoring theLiveOptionsclass for improved maintainability.The
LiveOptionsclass has a large number of optional fields, which might make it difficult to use and maintain. Consider the following improvements:
- Group related fields into nested dataclasses to improve organization and readability.
- Use a builder pattern or factory methods to create
LiveOptionsinstances with specific configurations.- Implement property methods for fields that require additional validation or transformation logic.
Additionally, the
checkmethod modifies the logger level, which might have unintended side effects. Consider refactoring it to:
- Use a separate logger instance for this method to avoid affecting the global logger configuration.
- Return a list of warnings instead of logging them directly, allowing the caller to handle the warnings as needed.
Here's an example of how you might start refactoring the class:
from dataclasses import dataclass from typing import List, Optional @dataclass class AudioOptions: encoding: Optional[str] = None sample_rate: Optional[int] = None channels: Optional[int] = None @dataclass class TranscriptionOptions: model: str = "nova-2" language: Optional[str] = None punctuate: Optional[bool] = None # ... other transcription-related fields @dataclass class LiveOptions(BaseResponse): audio: AudioOptions = field(default_factory=AudioOptions) transcription: TranscriptionOptions = field(default_factory=TranscriptionOptions) # ... other top-level fields @classmethod def with_audio_config(cls, encoding: str, sample_rate: int, channels: int) -> 'LiveOptions': return cls(audio=AudioOptions(encoding=encoding, sample_rate=sample_rate, channels=channels)) def check(self) -> List[str]: warnings = [] if self.transcription.tier: warnings.append("Tier is deprecated. Will be removed in a future version.") # ... other checks return warningsThis refactoring improves organization, reduces the number of top-level fields, and provides a more flexible way to create and validate
LiveOptionsinstances.deepgram/clients/__init__.py (1)
71-78: Approved with a minor suggestion: Consider naming consistencyThe changes effectively remove duplicate imports and add new source types, aligning with the PR objectives. However, there's a minor inconsistency in naming:
PreRecordedStreamSourceuses "RecOrded"PrerecordedSourceuses "recorded"Consider standardizing the capitalization for consistency, preferably to
PrerecordedStreamSource.deepgram/clients/listen/v1/rest/options.py (1)
173-178: Review new type alias and clarify comment.The addition of
PrerecordedSource = Union[UrlSource, FileSource]provides a clear type hint for accepted source types. This is a good improvement for code clarity.However, there's a potential issue with the comment on line 177-178:
# PrerecordedSource for backwards compatibility PreRecordedStreamSource = StreamSourceThis comment seems to be misplaced or incorrect, as it doesn't match the alias name
PreRecordedStreamSource.Consider updating the comment to accurately reflect the purpose of
PreRecordedStreamSource:# Retained for backwards compatibility PreRecordedStreamSource = StreamSourceAlso, consider adding a brief comment explaining the purpose of the new
PrerecordedSourcealias.deepgram/__init__.py (3)
48-61: LGTM: New response-related imports addedThe addition of these imports aligns well with the PR objective of exposing all types and classes at the top level. The logical grouping enhances code organization and readability.
Consider alphabetizing the imports within each group for even better organization and easier maintenance. For example:
from .client import ( Average, Intent, Intents, IntentsInfo, Segment, Sentiment, SentimentInfo, Sentiments, SummaryInfo, Topic, Topics, TopicsInfo, )
83-94: LGTM: WebSocket-related imports reorganized and expandedThe reorganization and addition of WebSocket-related imports enhance the SDK's structure and align with the PR objective. The categorization (top level, common websocket response, unique) improves code readability and organization.
Consider using a consistent comment style for better readability. For example:
# Top level LiveResultResponse, ListenWSMetadataResponse, SpeechStartedResponse, UtteranceEndResponse, # Common websocket response # OpenResponse, # CloseResponse, # UnhandledResponse, # ErrorResponse, # Unique ListenWSMetadata,
Line range hint
216-291: LGTM: Client-related imports reorganized and expandedThe reorganization and addition of client-related imports enhance the SDK's structure and align with the PR objective. The categorization (top level, shared) improves code readability and organization.
Consider alphabetizing the imports within each category for even better organization and easier maintenance. For example:
# Top level BalancesResponse, InvitesResponse, KeyResponse, KeysResponse, MembersResponse, Message, ModelResponse, ModelsResponse, ProjectsResponse, ScopesResponse, UsageFieldsResponse, UsageRequest, UsageRequestsResponse, UsageResponse, UsageSummaryResponse, # Shared Balance, Callback, Config, Invite, Key, Member, Project, Resolution, ...This alphabetical ordering will make it easier to locate specific imports and maintain the file in the future.
deepgram/client.py (3)
Line range hint
22-207: Improved import organization and new class additions.The reorganization of imports and addition of new classes significantly enhances code readability and aligns with the PR objective of exposing all types and classes at the top level of the response results. This change will make it easier for users to access any object in a response by its name or reference.
Consider using
__all__at the end of the file to explicitly define the public API. This would further clarify which classes and functions are intended to be used by external code.
Line range hint
300-451: Expanded DeepgramClient capabilities and API updates.The additions of
readandspeakproperties enhance the client's functionality, allowing for more comprehensive interaction with Deepgram's services. The deprecation notices forasyncspeak,onprem, andasynconpremproperties indicate a thoughtful evolution of the API design.Consider updating the class-level docstring for
DeepgramClientto include descriptions of the newreadandspeakproperties. This will help users understand the full range of available functionalities.
Line range hint
453-570: Version class implementation and future plans.The
Versionclass provides a flexible mechanism for version-specific client instantiation, which is crucial for maintaining backwards compatibility while allowing for future API updates. The commented-out code for potential future versioning shows foresight in API design.Consider adding a TODO comment or documentation note about the future versioning plans. This will help other developers understand the intended direction for API versioning and potentially contribute to its implementation.
deepgram/clients/speak/v1/rest/response.py (1)
Line range hint
24-29: Properly deprecate thestreamattributeThe
streamattribute is marked for deprecation in a future release, as indicated by the TODO comment. To clearly communicate this deprecation to users and developers, consider using thewarningsmodule to emit aDeprecationWarningwhen thestreamattribute is accessed. This will help users transition to usingstream_memoryand prevent unexpected issues whenstreamis eventually removed.Here is how you might implement this:
import warnings # ... @dataclass class SpeakRESTResponse(BaseResponse): # Existing fields... _stream: Optional[io.BytesIO] = field( default=None, metadata=dataclass_config(exclude=lambda f: True), ) @property def stream(self): warnings.warn( "The 'stream' attribute is deprecated and will be removed in a future release. Please use 'stream_memory' instead.", DeprecationWarning, stacklevel=2 ) return self._stream @stream.setter def stream(self, value): warnings.warn( "Setting the 'stream' attribute is deprecated and will be removed in a future release. Please use 'stream_memory' instead.", DeprecationWarning, stacklevel=2 ) self._stream = valuedeepgram/clients/analyze/v1/response.py (3)
Line range hint
47-58: Simplify the__getitem__method in theMetadataclass for improved efficiencyThe current implementation of the
__getitem__method converts the entire object to a dictionary and reconstructs certain fields, which may introduce unnecessary overhead. Consider simplifying the method to directly access attributes.Apply this diff to streamline the method:
def __getitem__(self, key): - _dict = self.to_dict() - if "intents_info" in _dict: - _dict["intents_info"] = IntentsInfo.from_dict(_dict["intents_info"]) - if "sentiment_info" in _dict: - _dict["sentiment_info"] = SentimentInfo.from_dict(_dict["sentiment_info"]) - if "summary_info" in _dict: - _dict["summary_info"] = SummaryInfo.from_dict(_dict["summary_info"]) - if "topics_info" in _dict: - _dict["topics_info"] = TopicsInfo.from_dict(_dict["topics_info"]) - return _dict[key] + return getattr(self, key)This change simplifies the method by directly accessing the attribute using
getattr, reducing overhead and improving performance.
Line range hint
71-81: Simplify the__getitem__method in theResultsclassSimilar to the
Metadataclass, the__getitem__method in theResultsclass can be optimized for better performance by directly accessing attributes.Apply this diff to simplify the method:
def __getitem__(self, key): - _dict = self.to_dict() - if "summary" in _dict: - _dict["summary"] = Summary.from_dict(_dict["summary"]) - if "sentiments" in _dict: - _dict["sentiments"] = Sentiments.from_dict(_dict["sentiments"]) - if "topics" in _dict: - _dict["topics"] = Topics.from_dict(_dict["topics"]) - if "intents" in _dict: - _dict["intents"] = Intents.from_dict(_dict["intents"]) - return _dict[key] + return getattr(self, key)This refactoring enhances code readability and reduces unnecessary processing.
Line range hint
96-105: Optimize the__getitem__method in theAnalyzeResponseclassTo maintain consistency and improve efficiency, consider simplifying the
__getitem__method in theAnalyzeResponseclass as well.Apply this diff:
def __getitem__(self, key): - _dict = self.to_dict() - if "metadata" in _dict: - _dict["metadata"] = Metadata.from_dict(_dict["metadata"]) - if "results" in _dict: - _dict["results"] = Results.from_dict(_dict["results"]) - return _dict[key] + return getattr(self, key)This change aligns the method with the simplified versions in other classes, improving overall consistency and performance.
deepgram/clients/listen/v1/websocket/response.py (1)
Line range hint
73-81: Refactor duplicate__getitem__methods to eliminate code duplication.The
__getitem__methods in bothLiveResultResponseandMetadataResponseclasses have similar implementations. To adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting this method into theBaseResponseclass or creating a utility function that both classes can utilize. This refactoring will reduce code redundancy and simplify future maintenance.Also applies to: 123-134
deepgram/clients/common/v1/rest_response.py (4)
5-5: Remove unused imports for cleaner codeThe imports
DictandAnyfrom thetypingmodule are not used in this file. Removing unused imports helps maintain code cleanliness and reduces potential confusion.Apply this diff to remove the unused imports:
-from typing import List, Optional, Dict, Any +from typing import List, Optional
8-8: Clarify aliasing of importedconfigfunctionYou have imported
configfromdataclasses_jsonasdataclass_config. This alias might be misleading since it suggests a relation to the standarddataclassesmodule. For clarity, consider using the original name or a different alias.Apply this diff to use the original name:
-from dataclasses_json import config as dataclass_config +from dataclasses_json import configAnd update all references accordingly.
17-59: Enhance class docstrings for better clarityThe docstrings for the
IntentsInfo,SentimentInfo,SummaryInfo, andTopicsInfoclasses currently only repeat the class names. Providing more descriptive docstrings improves code readability and helps other developers understand the purpose of each class.For example, update the docstring for
IntentsInfo:class IntentsInfo(BaseResponse): - """ - Intents Info - """ + """ + Contains metadata information for intents analysis, including model UUID and token counts. + """
1-181: Add comprehensive unit tests for new classesThe addition of multiple new data classes warrants thorough unit testing to ensure they behave as expected, especially given the custom
__getitem__methods and serialization logic.Would you like assistance in generating unit tests for these classes or to open a new GitHub issue to track this task?
deepgram/clients/manage/v1/options.py (1)
Line range hint
100-169: Consider refactoringUsageSummaryOptionsto improve maintainabilityThe
UsageSummaryOptionsclass has a large number of attributes (over 20), which may affect readability and maintainability. Consider grouping related fields into nested data classes or refactoring the class to adhere to the Single Responsibility Principle.deepgram/clients/listen/client.py (2)
88-101: Redundant aliasing of shared entitiesThe direct assignment of entities like
Average = AverageLatestmay be unnecessary if only one version is in use. Consider importing these entities without aliasing or implementing a versioning mechanism if multiple versions are expected.
Line range hint
118-121: Incorrect assignment ofAsyncLiveClientThe
AsyncLiveClientis currently assigned toListenWebSocketClientLatest, which is the synchronous client. This could cause confusion or errors when attempting to use asynchronous methods.Apply this diff to correct the assignment:
-AsyncLiveClient = ListenWebSocketClientLatest +AsyncLiveClient = AsyncListenWebSocketClientLatest
Comments failed to post (27)
deepgram/clients/speak/v1/__init__.py (1)
20-22:
⚠️ Potential issueRemove duplicate imports.
The imports on lines 20-22 are duplicates of the imports on lines 8-9. To maintain clean and efficient code, it's advisable to remove these duplicate imports.
Apply this diff to remove the duplicate imports:
-from .rest import ( - SpeakRESTOptions, - SpeakOptions, -)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.deepgram/clients/listen/v1/websocket/options.py (1)
13-17: 💡 Codebase verification
Base class
BaseResponsedoes not provide theto_dictmethod.Changing
LiveOptionsto inherit fromBaseResponseremoves theto_dictfunctionality, which may affect existing implementations that rely on this method. Please ensure that:
- The
to_dictmethod is no longer required forLiveOptions.- If
to_dictis needed, consider adding it toBaseResponseor implementing it withinLiveOptions.- Existing consumers of
LiveOptionsare updated to handle the absence of theto_dictmethod if necessary.Analysis chain
Verify the impact of changing the base class to
BaseResponse.The
LiveOptionsclass now inherits fromBaseResponseinstead ofDataClassJsonMixin. This change aligns with the PR objective of exposing all types at the top level. However, please ensure that:
BaseResponseprovides all necessary functionality previously offered byDataClassJsonMixin.- This change doesn't introduce any breaking changes for existing users of the
LiveOptionsclass.- The
@dataclassdecorator is still compatible with the new base class.To confirm the compatibility, run the following script:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and functionality of LiveOptions after the base class change. # Test 1: Check if LiveOptions is still being used as expected rg --type python -A 5 'LiveOptions' # Test 2: Verify that BaseResponse provides necessary functionality ast-grep --lang python --pattern 'class BaseResponse: $$$ def to_dict(self): $$$ $$$ 'Length of output: 221
Script:
#!/bin/bash # Description: Re-verify the usage and functionality of LiveOptions after the base class change without specifying file types. # Test 1: Check if LiveOptions is still being used as expected across all Python files rg 'LiveOptions' --glob '*.py' -A 5 # Test 2: Verify that BaseResponse provides necessary functionality by searching for the to_dict method ast-grep --lang python --pattern 'class BaseResponse: $$$ def to_dict(self): $$$ $$$ 'Length of output: 25238
deepgram/clients/__init__.py (1)
201-214:
⚠️ Potential issueApproved with a minor issue: Text-to-speech REST imports reorganization
The reorganization of text-to-speech REST imports is well-structured and aligns with the PR objectives. However, there's a minor issue to address:
- Lines 213-214 contain a duplicate import of
SpeakRESTSourcewith different capitalizations:SpeakRestSource, SpeakRESTSource,Please remove one of these imports, preferably keeping the one that matches the capitalization used elsewhere in the codebase (likely
SpeakRESTSource).Otherwise, the changes improve code organization and readability while expanding the SDK's capabilities.
deepgram/clients/speak/v1/websocket/response.py (1)
21-24: 🛠️ Refactor suggestion
Consider moving shared attributes to
BaseResponseThe
typeattribute is common across multiple response classes (MetadataResponse,FlushedResponse,ClearedResponse,WarningResponse). To adhere to the DRY principle and enhance maintainability, consider moving thetypeattribute to theBaseResponseclass.deepgram/clients/common/v1/shared_response.py (6)
123-128: 🛠️ Refactor suggestion
Optimize
__getitem__inSearchclass to improve performance.The
__getitem__method in theSearchclass also serializes and deserializes the entire object. Accessing attributes directly can enhance efficiency.Modify the method as follows:
def __getitem__(self, key): - _dict = self.to_dict() - if "hits" in _dict: - _dict["hits"] = [Hit.from_dict(hits) for hits in _dict["hits"]] - return _dict[key] + return getattr(self, key)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def __getitem__(self, key): return getattr(self, key)
141-150: 🛠️ Refactor suggestion
Optimize
__getitem__inChannelclass to improve performance.Similarly, the
__getitem__method in theChannelclass can be optimized to avoid unnecessary serialization.Modify the method to access attributes directly:
def __getitem__(self, key): - _dict = self.to_dict() - if "search" in _dict: - _dict["search"] = [Search.from_dict(search) for search in _dict["search"]] - if "alternatives" in _dict: - _dict["alternatives"] = [ - Alternative.from_dict(alternatives) - for alternatives in _dict["alternatives"] - ] - return _dict[key] + return getattr(self, key)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def __getitem__(self, key): return getattr(self, key)
95-100: 🛠️ Refactor suggestion
Optimize
__getitem__inAlternativeclass to improve performance.The
__getitem__method in theAlternativeclass serializes and deserializes the entire object, which may impact performance. Consider accessing attributes directly without converting to and from dictionaries.Modify the method to access attributes directly:
def __getitem__(self, key): - _dict = self.to_dict() - if "words" in _dict: - _dict["words"] = [Word.from_dict(words) for words in _dict["words"]] - return _dict[key] + return getattr(self, key)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def __getitem__(self, key): return getattr(self, key)
2-2:
⚠️ Potential issueCorrect grammatical error in the license header.
In the license comment on line 2, "a MIT license" should be "an MIT license" for grammatical correctness.
Apply this diff to fix 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.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Use of this source code is governed by an MIT license that can be found in the LICENSE file.
31-44:
⚠️ Potential issueConsider handling missing keys differently in the
get_nested_valuemethod.Currently, the method returns an empty string if the key is not found, which might not clearly indicate that the key is missing. Consider returning
Noneor raising aKeyErrorto better convey the absence of a value.Modify the method to return
Nonewhen a key is not found:else: - return "" + return NoneAlternatively, raise a
KeyError:else: - return "" + raise KeyError(f"Key '{key}' not found in the response object.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def eval(self, key: str) -> Optional[str]: """ This method is used to evaluate a key in the response object using a dot notation style method. """ keys = key.split(".") result: Dict[Any, Any] = self.to_dict() for k in keys: if isinstance(result, dict) and k in result: result = result[k] elif isinstance(result, list) and k.isdigit() and int(k) < len(result): result = result[int(k)] else: return None return str(result)
31-44: 🛠️ Refactor suggestion
Rename the
evalmethod to avoid shadowing built-in functions and potential confusion.The method
evalshadows Python's built-inevalfunction, which could lead to confusion or unintended behavior. Consider renaming it to something more descriptive likeget_nested_valueorresolve_key.Apply this diff to rename the method:
-def eval(self, key: str) -> str: +def get_nested_value(self, key: str) -> str: """ - This method is used to evaluate a key in the response object using a dot notation style method. + This method is used to retrieve a value from the response object using dot notation. """Also, update any internal references to this method accordingly.
Committable suggestion was skipped due to low confidence.
deepgram/clients/common/v1/rest_response.py (7)
17-181: 🛠️ Refactor suggestion
Consider abstracting redundant
__getitem__methodsThe
__getitem__methods in multiple classes (Average,Segment,Sentiments,Topics,Intents) share similar patterns. To adhere to the DRY (Don't Repeat Yourself) principle, consider creating a base class method or utility function to handle this logic.By abstracting this functionality, you reduce code duplication and simplify future maintenance.
70-75:
⚠️ Potential issueReview the implementation of
__getitem__method inAverageclassThe
__getitem__method attempts to convertsentimentusingSentiment.from_dict(). SinceSentimentis likely anEnum, it might not have afrom_dict()method, leading to anAttributeError.Consider removing or adjusting the conversion:
def __getitem__(self, key): _dict = self.to_dict() - if "sentiment" in _dict: - _dict["sentiment"] = Sentiment.from_dict(_dict["sentiment"]) return _dict[key]If custom deserialization is needed, ensure that
Sentimentprovides a suitable method.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def __getitem__(self, key): _dict = self.to_dict() return _dict[key]
62-69: 🛠️ Refactor suggestion
Reconsider default value for
sentiment_scoreIn the
Averageclass,sentiment_scoreis initialized to0. If a sentiment score of0is meaningful in your context, it's fine. However, if0could be confused with an uninitialized state, consider usingNoneas the default value to indicate the absence of a score.Adjust the default value:
sentiment_score: float = 0 + sentiment_score: Optional[float] = NoneDon't forget to import
Optionalif it's not already imported.Committable suggestion was skipped due to low confidence.
106-117:
⚠️ Potential issueIncorrect usage of
dataclass_configin field metadataIn the
Segmentclass, you usemetadata=dataclass_config(exclude=lambda f: f is None). Theexcludeparameter expects a boolean, not a function. This might not behave as intended during serialization.Use the correct syntax for excluding fields when they are
None:from dataclasses_json import config ... sentiment: Optional[Sentiment] = field( default=None, metadata=config(exclude=lambda x: x is None) )Ensure you import
configdirectly and adjust the lambda function parameter tox, representing the field's value.Committable suggestion was skipped due to low confidence.
140-148:
⚠️ Potential issueEnsure correctness of
__getitem__method inSentimentsclassSimilar to previous comments, check that
SegmentandAverageclasses have afrom_dict()method. The current implementation may result in errors if these methods do not exist.Review and adjust the code:
def __getitem__(self, key): _dict = self.to_dict() - if "segments" in _dict: - _dict["segments"] = [ - Segment.from_dict(segment) for segment in _dict["segments"] - ] - if "average" in _dict: - _dict["average"] = Average.from_dict(_dict["average"]) return _dict[key]Ensure that the deserialization process is correctly handled elsewhere if needed.
Committable suggestion was skipped due to low confidence.
158-165:
⚠️ Potential issueValidate
__getitem__method inTopicsclassEnsure that the
Segmentclass has afrom_dict()method if you intend to use it here. Otherwise, this code may raise anAttributeError.Adjust the method accordingly:
def __getitem__(self, key): _dict = self.to_dict() - if "segments" in _dict: - _dict["segments"] = [ - Segment.from_dict(segment) for segment in _dict["segments"] - ] return _dict[key]Provide proper deserialization if necessary.
Committable suggestion was skipped due to low confidence.
119-128:
⚠️ Potential issuePotential issues in
__getitem__method ofSegmentclassThe
__getitem__method tries to convert fields usingfrom_dict(), but it's unclear ifSentiment,Intent, andTopicclasses have this method. Additionally, modifying_dictmay lead to unexpected behavior.Verify that these classes have a
from_dict()method. If they do not, you may need to implement them or adjust the code accordingly.def __getitem__(self, key): _dict = self.to_dict() - if "sentiment" in _dict: - _dict["sentiment"] = Sentiment.from_dict(_dict["sentiment"]) - if "intents" in _dict: - _dict["intents"] = [Intent.from_dict(intent) for intent in _dict["intents"]] - if "topics" in _dict: - _dict["topics"] = [Topic.from_dict(topic) for topic in _dict["topics"]] return _dict[key]Consider whether overriding
__getitem__is necessary for your use case.Committable suggestion was skipped due to low confidence.
deepgram/clients/manage/v1/options.py (1)
173-174:
⚠️ Potential issueSet default values to
NoneforOptional[str]fieldsIn
UsageFieldsOptions, the fieldsstartandendare declared asOptional[str]but default to an empty string"". For consistency and to properly represent optional fields, it's preferable to set the default toNoneand usedataclass_configto exclude them when they areNone, as done in other classes.Apply this diff to update the defaults and metadata:
class UsageFieldsOptions(BaseResponse): """ Usage Fields Options """ - start: Optional[str] = "" - end: Optional[str] = "" + start: Optional[str] = field( + default=None, metadata=dataclass_config(exclude=lambda f: f is None) + ) + end: Optional[str] = field( + default=None, metadata=dataclass_config(exclude=lambda f: f is None) + )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class UsageFieldsOptions(BaseResponse): """ Usage Fields Options """ start: Optional[str] = field( default=None, metadata=dataclass_config(exclude=lambda f: f is None) ) end: Optional[str] = field( default=None, metadata=dataclass_config(exclude=lambda f: f is None) )deepgram/clients/listen/v1/rest/response.py (3)
351-351:
⚠️ Potential issueEnsure consistent naming conventions for class
ListenRESTChannelSimilar to previous classes, the class name
ListenRESTChannelshould follow the same naming pattern. Consider renaming it toListenRestChannelfor consistency.Apply this diff to correct the class name:
-class ListenRESTChannel(Channel): +class ListenRestChannel(Channel):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class ListenRestChannel(Channel):
252-252:
⚠️ Potential issueAvoid redefining the built-in
WarningclassDefining a class named
Warningcan overshadow Python's built-inWarningclass, leading to unexpected behavior. It's advisable to rename this class to prevent conflicts.Apply this diff to rename the class:
-class Warning(BaseResponse): # pylint: disable=used-before-assignment,redefined-builtin +class ListenRestWarning(BaseResponse):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class ListenRestWarning(BaseResponse):
308-310:
⚠️ Potential issueEnsure consistent naming conventions for class
ListenRESTAlternativeThe class name
ListenRESTAlternativeuses uppercaseREST, whereasListenRestWordusesRestwith only the 'R' capitalized. For consistency and readability, consider renaming the class toListenRestAlternative.Apply this diff to correct the class name:
-class ListenRESTAlternative( +class ListenRestAlternative(Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.class ListenRestAlternative( Alternative ): # pylint: disable=too-many-instance-attributesdeepgram/clients/manage/v1/response.py (6)
10-12: 🛠️ Refactor suggestion
Consider using absolute imports for clarity
Using triple-dot (
...) relative imports can be less clear and more error-prone, especially if the module hierarchy changes in the future. Consider using absolute imports to enhance readability and maintainability.
61-63: 🛠️ Refactor suggestion
Avoid disabling pylint warnings; consider refactoring
STTDetailsThe comment
# pylint: disable=too-many-instance-attributessuppresses a pylint warning about having too many instance attributes in theSTTDetailsclass. To improve code quality, consider refactoring the class to reduce complexity, possibly by grouping related attributes into nested classes or using composition.
84-86:
⚠️ Potential issueInconsistency between class name
TTSMetadataand its usageThe class
TTSMetadatais named as if it's specific to TTS models, but the docstring mentions it's used for both STT and TTS models. This could cause confusion.Consider one of the following actions:
- Rename the class to something more general like
ModelMetadataif it represents metadata for both STT and TTS models.- Update the docstring to accurately reflect that
TTSMetadatais specific to TTS models if that's the case.
110-110:
⚠️ Potential issueInconsistency in
metadatatype between declaration and usage inTTSDetailsThe
metadatafield is declared asOptional[TTSMetadata], indicating it should be either aTTSMetadatainstance orNone. However, in the__getitem__method,metadatais being iterated over as if it were a list:if "metadata" in _dict: _dict["metadata"] = [ TTSMetadata.from_dict(metadata) for metadata in _dict["metadata"] ]This inconsistency could lead to runtime errors.
Recommendation:
If
metadatashould be a single instance, modify the__getitem__method to handle it as such:if "metadata" in _dict: _dict["metadata"] = TTSMetadata.from_dict(_dict["metadata"])If
metadatashould be a list, update the field declaration:metadata: Optional[List[TTSMetadata]] = field(...)
138-138:
⚠️ Potential issueInconsistency in
metadatatype between declaration and usage inModelResponseSimilar to the issue in
TTSDetails, themetadatafield inModelResponseis declared asOptional[TTSMetadata], but used as a list in the__getitem__method:if "metadata" in _dict: _dict["metadata"] = [ TTSMetadata.from_dict(metadata) for metadata in _dict["metadata"] ]Recommendation:
- Align the field type and usage by either changing the field to a list or adjusting the
__getitem__method to handle a single instance.
352-352: 🛠️ Refactor suggestion
Avoid disabling pylint warnings; consider refactoring
STTUsageDetailsThe
# pylint: disable=too-many-instance-attributescomment indicates thatSTTUsageDetailshas too many attributes. Refactoring the class could improve maintainability.Suggestions:
- Group related attributes into nested data classes.
- Use dictionaries or other data structures to encapsulate related properties.
|
Units tests pass as well! |
davidvonthenen
force-pushed
the
issue457-expose-all-response-types
branch
from
b2c6dce to
7973bcb
Compare
Proposed changes
Addresses: #457
This exposes all types/classes in the top-level result results. Users should be able to access any object in a response by name or ref. This explicitly lists the objects and also explicitly state which ones are being used as "common" objects; you can see these as commented out items that are contained in the imports. This is to easily see what is being exported where. Example:
This is backward compatible, and no modification of client code is required. I manually ran each example to ensure the appropriate response.
As a consequence of doing this PR, there was a (much needed) consolidation of the BaseResponse class. This in turn deleted a bunch of code that is now inherited.
Created a test build
3.7.0-dev.2to verify all types are exposed. This checks out.Types of changes
What types of changes does your code introduce to the community Python 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
SpeakWSMetadataResponseto improve WebSocket response handling.Refactor
BaseResponsefor a more consistent response structure.Bug Fixes
Chores