🧹 Refactor _retry_request to improve readability#796
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 |
![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 → 9.10 | Complex Method, Complex Conditional, Bumpy Road Ahead, 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}
| if attempt < max_retries - 1: | ||
| time.sleep(wait_seconds) | ||
| return True | ||
| raise e # Max retries exceeded |
There was a problem hiding this comment.
📝 Info: raise e vs bare raise changes traceback display
The extracted helpers _handle_rate_limit (line 273) and _check_client_error (line 288) use raise e instead of the original bare raise. In Python 3, bare raise inside an except block preserves the original traceback, while raise e in a called function creates a new traceback starting from the raise e site. The exception object and its __context__ chain are preserved, so the behavior is functionally identical — but stack traces in logs or crash reports will now point to _handle_rate_limit:273 or _check_client_error:288 instead of directly to the _retry_request except block. This is a cosmetic difference that could slightly complicate debugging but is not a bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
| wait_seconds: int | None = None | ||
| with contextlib.suppress(ValueError): | ||
| wait_seconds = int(retry_after) | ||
|
|
||
| if wait_seconds is not None: | ||
| log.warning( | ||
| f"Rate limited (429). Server requests {wait_seconds}s wait " | ||
| f"(attempt {attempt + 1}/{max_retries})" | ||
| ) | ||
| if attempt < max_retries - 1: | ||
| time.sleep(wait_seconds) | ||
| return True | ||
| raise e # Max retries exceeded | ||
|
|
||
| return False |
There was a problem hiding this comment.
📝 Info: 429 with non-integer Retry-After falls through correctly
When the server sends a 429 with a Retry-After header that cannot be parsed as an integer (e.g. an HTTP-date format like Wed, 21 Oct 2015 07:28:00 GMT), _handle_rate_limit suppresses the ValueError from int(), wait_seconds stays None, and the function returns False. This causes the code to fall through to _check_client_error, which skips 429 (due to code != 429), and then to the standard exponential backoff retry. This matches the old behavior exactly. However, note that HTTP-date format Retry-After headers are never honored — the code only supports integer (seconds) format. This is a pre-existing limitation, not introduced by this PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _get_error_hint(e: Exception) -> str: | ||
| """Generate actionable error hints for logged exceptions.""" | ||
| if isinstance(e, httpx.TimeoutException): | ||
| return f" | hint: {_TIMEOUT_HINT}" | ||
| if isinstance(e, httpx.ConnectError): | ||
| return f" | hint: {_CONNECT_ERROR_HINT}" | ||
| if ( | ||
| isinstance(e, httpx.HTTPStatusError) | ||
| and hasattr(e, "response") | ||
| and e.response is not None | ||
| and e.response.status_code >= 500 | ||
| ): | ||
| return f" | hint: {_SERVER_ERROR_HINT}" | ||
| return "" | ||
|
|
||
|
|
||
| def _log_debug_response_content(e: Exception) -> None: | ||
| """Log response content at debug level if available.""" | ||
| if ( | ||
| hasattr(e, "response") | ||
| and getattr(e, "response", None) is not None | ||
| and log.isEnabledFor(logging.DEBUG) | ||
| ): | ||
| log.debug(f"Response content: {_sanitize_fn(e.response.text)}") | ||
|
|
||
|
|
||
| def _handle_rate_limit( | ||
| e: httpx.HTTPStatusError, attempt: int, max_retries: int | ||
| ) -> bool: | ||
| """ | ||
| Handle 429 Too Many Requests by waiting if Retry-After is present. | ||
|
|
||
| Returns: | ||
| True if the request should be retried immediately (after sleeping). | ||
| False if no Retry-After header was found (should use default backoff). | ||
| """ | ||
| if e.response.status_code != 429: | ||
| return False | ||
|
|
||
| retry_after = e.response.headers.get("Retry-After") | ||
| if not retry_after: | ||
| return False | ||
|
|
||
| wait_seconds: int | None = None | ||
| with contextlib.suppress(ValueError): | ||
| wait_seconds = int(retry_after) | ||
|
|
||
| if wait_seconds is not None: | ||
| log.warning( | ||
| f"Rate limited (429). Server requests {wait_seconds}s wait " | ||
| f"(attempt {attempt + 1}/{max_retries})" | ||
| ) | ||
| if attempt < max_retries - 1: | ||
| time.sleep(wait_seconds) | ||
| return True | ||
| raise e # Max retries exceeded | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def _check_client_error(e: httpx.HTTPStatusError) -> None: | ||
| """Raise exception for 4xx client errors (excluding 429) without retrying.""" | ||
| code = e.response.status_code | ||
| if 400 <= code < 500 and code != 429: | ||
| hint = _4XX_HINTS.get(code, "") | ||
| hint_suffix = f" | hint: {hint}" if hint else "" | ||
| log.warning( | ||
| f"API request failed with HTTP {code}{hint_suffix}: {_sanitize_fn(e)}" | ||
| ) | ||
| _log_debug_response_content(e) | ||
| raise e | ||
|
|
There was a problem hiding this comment.
📝 Info: Extracted helpers not added to __all__
The four new helper functions (_get_error_hint, _log_debug_response_content, _handle_rate_limit, _check_client_error) are not exported in __all__ at api_client.py:36-54. This is consistent with the existing convention — they are underscore-prefixed internal helpers not intended for use by main.py. All existing __all__ entries are either public constants or functions directly called by main.py. So this is correct.
Was this helpful? React with 👍 or 👎 to provide feedback.
🎯 What: The
_retry_requestfunction inapi_client.pywas too long and complex. The logic inside the try/except block was extracted into smaller helper functions (_get_error_hint,_log_debug_response_content,_handle_rate_limit,_check_client_error).💡 Why: This improves the readability, modularity, and maintainability of
api_client.py. It also resolves the "Bumpy Road Ahead" / "Brain Method" code smell by reducing cyclomatic complexity.✅ Verification: I ran
uv tool run ruff format .,uv tool run ruff check .,uv run pytest, anduv tool run pre-commit run --all-files. All checks passed.✨ Result: A simplified
_retry_requestfunction, making it easier to parse and debug without altering its underlying logic.