MPT-15431 Add MPTMaxRetryError in the MPT Errors and error handler#316
Merged
MPT-15431 Add MPTMaxRetryError in the MPT Errors and error handler#316
Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
jentyk
force-pushed
the
feat/MPT-15431
branch
4 times, most recently
from
b4c0f6c to
ca9e41f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/http/test_async_client.py (1)
78-85: Consider usingMPTMaxRetryErrorfor consistency with the sync client test.The sync client test (
test_client.pyline 74) explicitly assertsMPTMaxRetryError, while this async test uses the baseMPTError. While functionally correct (due to inheritance), using the specific exception type would:
- Be consistent with the sync test
- More precisely verify the expected behavior
- Fail faster if the exception type changes
♻️ Proposed fix for test consistency
-from mpt_api_client.exceptions import MPTError +from mpt_api_client.exceptions import MPTMaxRetryError- with pytest.raises(MPTError, match=r"Mock Timeout error after 6 retry attempts."): + with pytest.raises(MPTMaxRetryError, match=r"Mock Timeout error after 6 retry attempts."):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/http/test_async_client.py` around lines 78 - 85, In test_async_http_call_failure, replace the generic MPTError in the pytest.raises assertion with the more specific MPTMaxRetryError to match the sync test; ensure you still await async_http_client.request("GET", "/timeout") inside the with pytest.raises(MPTMaxRetryError, match=r"Mock Timeout error after 6 retry attempts.") block so the test asserts the specific max-retry exception type.mpt_api_client/http/client.py (1)
91-95: Docstring missingMPTMaxRetryErrorin raised exceptions.The async client's docstring (line 85 in
async_client.py) includesMPTMaxRetryErrorin the Raises section, but the sync client's docstring does not. For consistency, add the same exception to the Raises section.📝 Proposed docstring update
Raises: MPTError: If the request fails. MPTApiError: If the response contains an error. MPTHttpError: If the response contains an HTTP error. + MPTMaxRetryError: If the request fails after maximum retry attempts. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/http/client.py` around lines 91 - 95, The sync client's docstring in mpt_api_client/http/client.py lists raised exceptions (MPTError, MPTApiError, MPTHttpError) but omits MPTMaxRetryError; update the Raises section of the sync client's docstring to include MPTMaxRetryError (mirroring the async client in async_client.py) so that the docstring for the relevant request method/client class reflects all possible exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mpt_api_client/http/client.py`:
- Around line 91-95: The sync client's docstring in
mpt_api_client/http/client.py lists raised exceptions (MPTError, MPTApiError,
MPTHttpError) but omits MPTMaxRetryError; update the Raises section of the sync
client's docstring to include MPTMaxRetryError (mirroring the async client in
async_client.py) so that the docstring for the relevant request method/client
class reflects all possible exceptions.
In `@tests/unit/http/test_async_client.py`:
- Around line 78-85: In test_async_http_call_failure, replace the generic
MPTError in the pytest.raises assertion with the more specific MPTMaxRetryError
to match the sync test; ensure you still await async_http_client.request("GET",
"/timeout") inside the with pytest.raises(MPTMaxRetryError, match=r"Mock Timeout
error after 6 retry attempts.") block so the test asserts the specific max-retry
exception type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 68a7645a-b7a5-4644-ae05-e6982792e3b7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
mpt_api_client/exceptions.pympt_api_client/http/async_client.pympt_api_client/http/client.pympt_api_client/http/request_response_utils.pypyproject.tomltests/unit/http/test_async_client.pytests/unit/http/test_client.py
albertsola
reviewed
Apr 21, 2026
albertsola
requested changes
Apr 21, 2026
albertsola
approved these changes
Apr 21, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/http/async_client.py`:
- Around line 27-30: The Retry initialization currently uses defaults that
exclude POST/PATCH; update the retry creation (the Retry(...) passed into
RetryTransport) to explicitly set allowed_methods to include POST and PATCH
(e.g., include "GET","POST","PATCH","PUT","DELETE","HEAD","OPTIONS","TRACE") so
POST and PATCH calls made via RetryTransport will be retried; change the
Retry(...) calls that create the retry object used by RetryTransport (the
variables named retry and transport in the Async client) to pass allowed_methods
accordingly.
In `@mpt_api_client/http/client.py`:
- Around line 37-40: The Retry created in the HTTP client uses default
allowed_methods which excludes POST/PATCH; update the Retry instantiation (where
self._retries is used to build Retry and passed to RetryTransport) to explicitly
include POST and PATCH in allowed_methods (e.g., a frozenset containing GET,
HEAD, OPTIONS, POST, PUT, PATCH, DELETE) so POST/PATCH requests will be retried
on transient failures; make the identical change in the async client where
Retry(total=self._retries) is constructed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8492fe7f-0b3c-41c3-8908-5005a2238d42
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
mpt_api_client/exceptions.pympt_api_client/http/async_client.pympt_api_client/http/client.pympt_api_client/http/request_response_utils.pypyproject.tomltests/unit/http/test_async_client.pytests/unit/http/test_client.py
✅ Files skipped from review due to trivial changes (2)
- mpt_api_client/exceptions.py
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/http/test_client.py
- tests/unit/http/test_async_client.py
- mpt_api_client/http/request_response_utils.py
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mpt_api_client/http/async_client.py (1)
27-31: Validateretriesupfront to fail fast on invalid config.Consider guarding against negative values (e.g.,
retries < 0) in__init__to prevent ambiguous runtime behavior from invalid client configuration.Suggested diff
def __init__( self, *, base_url: str | None = None, api_token: str | None = None, timeout: float = 20.0, retries: int = 5, ): + if retries < 0: + raise ValueError("retries must be greater than or equal to 0") self._retries = retries retry = Retry( total=retries, allowed_methods={"DELETE", "GET", "HEAD", "OPTIONS", "POST", "PUT", "PATCH"}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/http/async_client.py` around lines 27 - 31, In __init__ of the async client (where self._retries is set and Retry(...) is constructed), validate the retries parameter up-front: ensure it's an integer and non-negative (e.g., raise ValueError for retries < 0 or TypeError if not int) before assigning to self._retries and creating the Retry object; this enforces fail-fast behavior and prevents ambiguous runtime config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mpt_api_client/http/async_client.py`:
- Around line 27-31: In __init__ of the async client (where self._retries is set
and Retry(...) is constructed), validate the retries parameter up-front: ensure
it's an integer and non-negative (e.g., raise ValueError for retries < 0 or
TypeError if not int) before assigning to self._retries and creating the Retry
object; this enforces fail-fast behavior and prevents ambiguous runtime config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a33017fd-7463-4ee1-a431-77f378ca0b94
📒 Files selected for processing (6)
mpt_api_client/exceptions.pympt_api_client/http/async_client.pympt_api_client/http/client.pympt_api_client/http/request_response_utils.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/http/test_async_client.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/http/test_client.py
- mpt_api_client/exceptions.py
- mpt_api_client/http/request_response_utils.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-15431