util: Fix decoding msgpack encoded error responses #571
Conversation
Signed-off-by: Lewis Marshall <[email protected]>
WalkthroughThe changes introduced in the Changes
Assessment against linked issues
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ably/util/exceptions.py (3 hunks)
🧰 Additional context used
🪛 Ruff
ably/util/exceptions.py
44-46: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🔇 Additional comments (2)
ably/util/exceptions.py (2)
41-44: 🛠️ Refactor suggestionUse exception chaining to preserve the original exception context
Capturing the exception as
eand using exception chaining withfrom eretains the original traceback, which aids in debugging.Apply this diff to modify the exception handling:
except Exception: log.debug("Response not json or msgpack: %d %s", response.status_code, response.text) - raise AblyException(message=response.text, - status_code=response.status_code, - code=response.status_code * 100) + raise AblyException(message=response.text, + status_code=response.status_code, + code=response.status_code * 100) from eLikely invalid or redundant comment.
3-3: Ensure 'msgpack' is included in the project dependenciesSince
msgpackis being imported, please verify that it is listed in the project's dependencies to avoid import errors.Run the following script to check if
msgpackis included insetup.pyorrequirements.txt:✅ Verification successful
'msgpack' is successfully listed in the project dependencies within
pyproject.toml.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'msgpack' is listed in the project dependencies. # Test: Search for 'msgpack' in dependency files. Expect: 'msgpack' should be listed. grep -H 'msgpack' setup.py requirements.txtLength of output: 135
Script:
#!/bin/bash # Description: Verify that 'msgpack' is listed in the project dependencies across various dependency files. # Define an array of potential dependency files dependency_files=("pyproject.toml" "Pipfile" "environment.yml" "tox.ini" "setup.cfg") # Initialize a flag to track if 'msgpack' is found found=0 # Iterate over each dependency file and search for 'msgpack' for file in "${dependency_files[@]}"; do if [ -f "$file" ]; then echo "Searching in $file:" grep -i 'msgpack' "$file" && found=1 fi done # If 'msgpack' was not found in any of the files if [ $found -eq 0 ]; then echo "'msgpack' not found in any recognized dependency files." fiLength of output: 646
|
We should also take a look at coderabbit suggestions |
|
Forgot to comment, we should have added integration test for this |
Existing tests were failing, so this is already covered, it's just that the tests don't run regularly, so no one noticed they started to fail. |
Ohkay, got it, thanks ! |
This updates
AblyException.raise_for_responseto handle msgpack encoded responses, much likeResponse.to_native:ably-python/ably/http/http.py
Lines 100 to 107 in 4066ce3
Fixes #569.
Summary by CodeRabbit
New Features
Bug Fixes