🧹 Simplify deeply nested logic in _retry_request#807
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
PR SummaryLow Risk Overview This keeps the existing retry/backoff behavior but centralizes the repeated parsing/logging logic to improve readability and maintainability. Reviewed by Cursor Bugbot for commit 330c7b8. Configure here. |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| api_client.py | 7.43 → 7.47 | Complex Method, Complex Conditional, Overall Code Complexity, Deep, Nested Complexity |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| retry_after = response.headers.get("Retry-After") | ||
| if not retry_after: | ||
| return None | ||
| import contextlib |
There was a problem hiding this comment.
📝 Info: Redundant import contextlib inside _get_retry_after_seconds
contextlib is already imported at module level (api_client.py:23), so the local import contextlib at line 233 is unnecessary. Python caches module imports so there's no correctness issue, but it's inconsistent with the rest of the file (e.g., _extract_int_header at api_client.py:119 uses the top-level import directly). This appears to be a leftover from copy-paste or an attempt at lazy importing that doesn't apply here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| and log.isEnabledFor(logging.DEBUG) | ||
| ): | ||
| log.debug(f"Response content: {_sanitize_fn(e.response.text)}") | ||
| _log_debug_response(e) |
There was a problem hiding this comment.
📝 Info: Inconsistent use of _log_debug_response helper — only applied to one of two identical patterns
The new _log_debug_response helper replaces the inline debug-logging pattern at the 4xx error branch (line 302), but the identical pattern at the max-retries exhaustion branch (api_client.py:306-311) was left as inline code. Both perform the same hasattr/getattr/isEnabledFor check and log.debug call. Using the helper in both places would improve consistency and reduce duplication. Not a bug since behavior is preserved, but a missed opportunity in the refactoring.
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: Simplified deeply nested error handling logic within
_retry_requestinapi_client.pyby extracting repetitive code into helper functions_log_debug_responseand_get_retry_after_seconds.💡 Why: This reduces cognitive complexity and cyclomatic complexity of
_retry_request, improving readability and maintainability without altering existing retry and logging functionality.✅ Verification: Verified the logic structurally. Formatted and checked the module with
ruff. Ran the full test suite (uv run pytest) and pre-commit hooks to confirm no regressions or formatting issues were introduced.✨ Result: A cleaner, flatter retry loop that relies on modular, specific helper functions.