🧹 Refactor _retry_request to improve readability#803
Conversation
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
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 |
github-actions
Bot
added
documentation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 CONTRIBUTING.md requires CHANGELOG.md update for every PR
CONTRIBUTING.md section 'Submitting a Pull Request' item 4 states: 'Update CHANGELOG.md — add an entry under the [Unreleased] section.' This PR does not include a CHANGELOG.md update. While this is a process requirement rather than a code defect, reviewers may want to request a changelog entry describing the _retry_request refactoring before merging.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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: Helper functions use raise e instead of bare raise, altering tracebacks
The extracted helpers _handle_rate_limit (api_client.py:273) and _check_client_error (api_client.py:288) use raise e instead of the original bare raise. This is functionally equivalent (same exception type and message propagate), but it changes the traceback shape. Bare raise preserves the original traceback (1 frame), while raise e from a helper produces a 3-frame traceback that includes the helper function. Verified empirically: raise e from a helper called within an except block produces output like:
File ..., in _retry_request
_check_client_error(e)
File ..., in _check_client_error
raise e
File ..., in _retry_request
response.raise_for_status()
This makes error tracebacks slightly noisier but doesn't affect exception propagation or error handling. No __context__ circular chain issue occurs since Python detects the self-reference. Not a bug, but worth noting since the PR claims 'without altering its underlying logic' — the observable traceback output is different.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
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.
| 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 "" |
There was a problem hiding this comment.
📝 Info: httpx exception hierarchy confirms _get_error_hint check ordering is safe
The _get_error_hint function checks TimeoutException before ConnectError before HTTPStatusError. I verified the installed httpx version's hierarchy: ConnectTimeout inherits from TimeoutException but NOT from ConnectError (they share TransportError as a common ancestor). This means a ConnectTimeout will correctly match the TimeoutException branch first, which is the same behavior as the old elif chain. No ordering issue exists.
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 unparseable Retry-After falls through correctly to backoff
When a 429 response has a Retry-After header that isn't a valid integer (e.g., an HTTP-date like Thu, 01 Dec 2022 16:00:00 GMT), the contextlib.suppress(ValueError) at line 262 catches the parse failure, wait_seconds remains None, and _handle_rate_limit returns False. Then _check_client_error skips 429 (line 281: code != 429), and the request falls through to normal exponential backoff. This matches the old code's behavior exactly.
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.PR created automatically by Jules for task 7929654001285434217 started by @abhimehro