Fix get_usage_request when Response Not Available
#473
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/edge_cases/usage_to_fast/main.py (2)
1-22: Consider making the audio file path configurable.The
AUDIO_FILEconstant is hardcoded, which might limit the script's flexibility. Consider making it configurable through environment variables or command-line arguments for easier testing with different audio files.
32-44: Improve project selection logic.The script currently selects the first project from the list, which might not always be the desired behavior. Consider adding logic to select a specific project based on criteria or allow user input for project selection.
deepgram/clients/manage/v1/client.py (1)
1007-1014: LGTM: Improved error handling for unavailable responses.The new error handling logic effectively addresses the issue mentioned in the PR objectives. It checks for the absence of a 'response' field and raises a
DeepgramErrorwith an informative message when the response is not available.A minor suggestion for improvement:
Consider using a more specific exception name, such as
DeepgramResponseNotAvailableError, to provide more context about the specific error condition. This would allow users to catch and handle this specific error case if needed.- if json_result.get("response") is None: - raise DeepgramError( - "Response is not available yet. Please try again later." - ) + if json_result.get("response") is None: + raise DeepgramResponseNotAvailableError( + "Response is not available yet. Please try again later." + )Don't forget to add the new exception class to the appropriate module if you decide to implement this suggestion.
deepgram/clients/manage/v1/async_client.py (1)
1004-1011: LGTM: Improved error handling for unavailable responses.The added code effectively addresses the issue mentioned in GitHub issue #457. It properly checks for the presence of the "response" field and raises a
DeepgramErrorwith a clear message when the response is not available.A minor suggestion for improvement:
Consider using a constant for the error message to improve maintainability. For example:
RESPONSE_NOT_AVAILABLE_ERROR = "Response is not available yet. Please try again later." # In the method: if json_result.get("response") is None: raise DeepgramError(RESPONSE_NOT_AVAILABLE_ERROR)This change would make it easier to update the error message in the future if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- deepgram/clients/manage/v1/async_client.py (2 hunks)
- deepgram/clients/manage/v1/client.py (2 hunks)
- tests/edge_cases/usage_to_fast/main.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
tests/edge_cases/usage_to_fast/main.py (1)
1-83: Overall assessment: Good test script with room for improvement.This script serves its purpose as a test case for the Deepgram API, demonstrating the usage of various SDK features. The structure is clear, and it includes basic error handling. However, there are several areas where it can be improved:
- Make the audio file path configurable.
- Use environment variables for sensitive information like the API key.
- Improve project selection logic.
- Add error handling for file reading.
- Implement a polling mechanism for usage request retrieval.
Implementing these suggestions will make the script more robust, flexible, and suitable for various testing scenarios.
deepgram/clients/manage/v1/client.py (3)
7-7: LGTM: New imports for enhanced error handling.The addition of
jsonandDeepgramErrorimports is appropriate for the new error handling logic in theget_usage_requestmethod.Also applies to: 13-13
Line range hint
1-1214: Summary: Effective implementation of error handling for unavailable responses.The changes in this PR successfully address the issue of handling unavailable responses in the
get_usage_requestmethod. The implementation is clean, focused, and consistent with the existing code style.Key points:
- New imports (
jsonandDeepgramError) support the enhanced error handling.- The
get_usage_requestmethod now checks for the presence of the 'response' field and raises an informative error when it's missing.These changes improve the robustness of the SDK and provide better feedback to users when responses are not immediately available.
Suggestions for consideration:
- Using a more specific exception name for clearer error handling.
- Reviewing other API-interacting methods for potential similar error handling needs.
Overall, this is a valuable improvement to the Deepgram Python SDK.
1007-1014: Verify if similar error handling is needed in other methods.The new error handling for unavailable responses is a good addition to the
get_usage_requestmethod. It might be beneficial to review other methods in this class that interact with the API to see if they would benefit from similar error handling.To help with this verification, you can run the following script:
This script will help identify other methods that use
self.getand process JSON responses, which might benefit from similar error handling.deepgram/clients/manage/v1/async_client.py (2)
7-7: LGTM: Import changes are appropriate.The addition of the
jsonimport and the inclusion ofDeepgramErrorin the import from...commonare well-justified changes. These modifications support the new error handling in theget_usage_requestmethod.Also applies to: 13-13
7-7: Summary: Effective implementation of error handling for unavailable responses.The changes in this file successfully address the issue described in GitHub issue #457. The addition of JSON parsing and checking for the "response" field in the
get_usage_requestmethod improves the robustness of the SDK by explicitly handling cases where the response is not yet available.Key improvements:
- Added
jsonimport for parsing the response.- Included
DeepgramErrorin the import from...common.- Implemented error checking and raising of
DeepgramErrorwhen the response is unavailable.These changes enhance the user experience by providing clear feedback when a response is not ready, aligning well with the PR objectives.
Also applies to: 13-13, 1004-1011
Proposed changes
Address: #457
Implement example to expose error.
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
Bug Fixes
Documentation