-
Notifications
You must be signed in to change notification settings - Fork 0
MPT-14890 Tests for catalog/product/media #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c9c7f2a to
1375b24
Compare
1375b24 to
1ac1861
Compare
mpt_api_client/http/async_client.py
Outdated
|
|
||
| from mpt_api_client.constants import APPLICATION_JSON | ||
| from mpt_api_client.exceptions import MPTError, transform_http_status_exception | ||
| from mpt_api_client.http.mixins import _json_to_file_payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we importing a private method here? I wonder why the PLC2701 wasn't raised by the linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we are moving that functionality from mixins to client. Temporary is in both places.
I have moved it to the http.client now.
@robcsegal will refactor the rest of the mixins to remove that dependency.
mpt_api_client/http/mixins.py
Outdated
|
|
||
|
|
||
| def _json_to_file_payload(resource_data: ResourceData) -> bytes: | ||
| def _json_to_file_payload(resource_data: ResourceData | None) -> bytes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this private method outside of the class? I believe the responsibility for that belongs to the FilesOperationsMixin. If you don't want to duplicate the code you should add a parent class with this method, so it can be inherited by the Async and sync mixins. Or, you can duplicate the code to keep the sync and async logic separated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are removing FilesOperationMixin, we are moving all files handling logic into the http.client.
Otherwise each request with a file upload is a copy of FilesOperationMixin with subsequent unit tests.
1ac1861 to
635f194
Compare
635f194 to
1751a85
Compare
|


No description provided.