Merged
Conversation
📝 WalkthroughWalkthroughMonolithic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mpt_api_client/http/mixins/disable_mixin.py`:
- Around line 18-20: The disable method's docstring contains an extra space
before the period; update the docstring in the disable function (disable) to
remove the double space so it reads "Disable a specific resource." ensuring the
triple-quoted string in disable (in disable_mixin.py) is edited accordingly.
In `@tests/unit/http/mixins/test_disable_mixin.py`:
- Around line 36-97: The tests test_disable_resource_actions and
test_async_disable_resource_actions assert an empty body when input_status is
None, but calling disablable_service.disable / async_disablable_service.disable
with json=None produces a body of b'null'; update both tests to expect b'null'
for the None case (i.e., set request_expected_content =
b'{"id":"OBJ-0000-0001","status":"update"}' if input_status else b'null') so the
assertions match the actual request.content returned by the mocked httpx call.
🧹 Nitpick comments (5)
mpt_api_client/http/mixins/file_operations_mixin.py (2)
17-17: Remove unusednoqadirective.Static analysis indicates
# noqa: WPS221is no longer needed here.🧹 Proposed fix
- files: dict[str, FileTypes] | None = None, # noqa: WPS221 + files: dict[str, FileTypes] | None = None,
49-49: Remove unusednoqadirective.Same as above - the
WPS221directive is no longer applicable.🧹 Proposed fix
- files: dict[str, FileTypes] | None = None, # noqa: WPS221 + files: dict[str, FileTypes] | None = None,tests/unit/http/mixins/test_get_mixin.py (1)
31-60: Consider consolidating async get tests with parametrization.The two async tests (
test_async_getandtest_async_get_select_str) could be combined into a single parametrized test similar totest_sync_get_mixin, reducing duplication. However, the current structure is clear and functional.♻️ Optional consolidation
+@pytest.mark.parametrize( + "select_value", + [ + ["id", "name"], + "id,name", + ], +) +async def test_async_get_mixin( + async_dummy_service: AsyncDummyService, select_value: str | list[str] +) -> None: + """Test getting a resource asynchronously with different select parameter formats.""" + resource_data = {"id": "RES-123", "name": "Test Resource"} + with respx.mock: + mock_route = respx.get( + "https://api.example.com/api/v1/test/RES-123", params={"select": "id,name"} + ).mock(return_value=httpx.Response(httpx.codes.OK, json=resource_data)) + + result = await async_dummy_service.get("RES-123", select=select_value) + + request = mock_route.calls[0].request + accept_header = (b"Accept", b"application/json") + assert accept_header in request.headers.raw + assert result.to_dict() == resource_data - -async def test_async_get(async_dummy_service: AsyncDummyService) -> None: - """Test getting a resource asynchronously with a list select parameter.""" - resource_data = {"id": "RES-123", "name": "Test Resource"} - with respx.mock: - mock_route = respx.get( - "https://api.example.com/api/v1/test/RES-123", params={"select": "id,name"} - ).mock(return_value=httpx.Response(httpx.codes.OK, json=resource_data)) - - result = await async_dummy_service.get("RES-123", select=["id", "name"]) - - request = mock_route.calls[0].request - accept_header = (b"Accept", b"application/json") - assert accept_header in request.headers.raw - assert result.to_dict() == resource_data - - -async def test_async_get_select_str(async_dummy_service: AsyncDummyService) -> None: - """Test getting a resource asynchronously with a string select parameter.""" - resource_data = {"id": "RES-123", "name": "Test Resource"} - with respx.mock: - mock_route = respx.get( - "https://api.example.com/api/v1/test/RES-123", params={"select": "id,name"} - ).mock(return_value=httpx.Response(httpx.codes.OK, json=resource_data)) - - result = await async_dummy_service.get("RES-123", select="id,name") - - request = mock_route.calls[0].request - accept_header = (b"Accept", b"application/json") - assert accept_header in request.headers.raw - assert result.to_dict() == resource_datampt_api_client/http/mixins/resource_mixins.py (1)
7-24: Tighten docstring wording.There’s repeated wording (“resource resources”) in multiple docstrings. Consider tightening for clarity.
✍️ Proposed cleanup
-class ModifiableResourceMixin[Model](GetMixin[Model], UpdateMixin[Model], DeleteMixin): - """Editable resource mixin allows to read and update a resource resources.""" +class ModifiableResourceMixin[Model](GetMixin[Model], UpdateMixin[Model], DeleteMixin): + """Editable resource mixin allows reading and updating resources.""" @@ -class AsyncModifiableResourceMixin[Model]( +class AsyncModifiableResourceMixin[Model]( AsyncGetMixin[Model], AsyncUpdateMixin[Model], AsyncDeleteMixin ): - """Editable resource mixin allows to read and update a resource resources.""" + """Editable resource mixin allows reading and updating resources.""" @@ -class ManagedResourceMixin[Model](CreateMixin[Model], ModifiableResourceMixin[Model]): - """Managed resource mixin allows to read, create, update and delete a resource resources.""" +class ManagedResourceMixin[Model](CreateMixin[Model], ModifiableResourceMixin[Model]): + """Managed resource mixin allows reading, creating, updating, and deleting resources.""" @@ -class AsyncManagedResourceMixin[Model]( +class AsyncManagedResourceMixin[Model]( AsyncCreateMixin[Model], AsyncModifiableResourceMixin[Model] ): - """Managed resource mixin allows to read, create, update and delete a resource resources.""" + """Managed resource mixin allows reading, creating, updating, and deleting resources."""tests/unit/http/mixins/test_resource_mixins.py (1)
36-36: Fix test name typo for clarity.
test_modifieable_resource_mixinis misspelled; consider renaming totest_modifiable_resource_mixin.✏️ Proposed rename
-def test_modifieable_resource_mixin(method_name: str) -> None: +def test_modifiable_resource_mixin(method_name: str) -> None:
albertsola
force-pushed
the
MPT-16437/Reorganise-mixins
branch
from
fd7377f to
92149de
Compare
|
d3rky
approved these changes
Feb 2, 2026
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-16437