fix: error response cleansing#51
Conversation
📝 WalkthroughWalkthroughThe PR adds error serialization support and payload normalization to the error-handling module. A new ChangesError handling and serialization enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_errors.py (1)
464-468: 💤 Low valueRedundant status code assignment.
Line 466 sets
mock_resp.status_code = 204but this is already set by_make_mock_response(204, ...)on line 465.♻️ Suggested fix
def test_204_returns_none(self, resource: BaseResource) -> None: mock_resp = self._make_mock_response(204, "No Content") - mock_resp.status_code = 204 result = resource._handle_response(mock_resp) assert result is None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_errors.py` around lines 464 - 468, Remove the redundant status assignment in test_204_returns_none: the helper _make_mock_response(204, "No Content") already sets mock_resp.status_code, so delete the explicit mock_resp.status_code = 204 line in the test (test_204_returns_none) and keep the call to _make_mock_response and the subsequent _handle_response assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_errors.py`:
- Around line 464-468: Remove the redundant status assignment in
test_204_returns_none: the helper _make_mock_response(204, "No Content") already
sets mock_resp.status_code, so delete the explicit mock_resp.status_code = 204
line in the test (test_204_returns_none) and keep the call to
_make_mock_response and the subsequent _handle_response assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01167552-096e-4d63-b5b8-d32c4c0d070b
📒 Files selected for processing (2)
plane/errors/errors.pytests/unit/test_errors.py
Description
Surface API error details (field-level validation, detail/error keys) in
HttpError.__str__and tracebacks instead of suppressing them behind a genericHTTP 400: Bad Requestmessage.Summary by CodeRabbit
Refactor
Tests