-
Notifications
You must be signed in to change notification settings - Fork 6
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
refactor: introduce async method and wip inline types #100
Conversation
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.
fingerprint-pro-server-api-php-sdk/src/Api/FingerprintApi.php
Lines 140 to 145 in ee54f5b
$data = ObjectSerializer::deserialize( | |
$responseBody, | |
'\Fingerprint\ServerAPI\Model\EventResponse', | |
$response->getHeaders() | |
); | |
$e->setResponseObject($data); |
Is this correct behavior? Here we set $data which is parsed response body, but $responseObject property in this class is
ResponseInterface
. We probably need to introduce another property that will store this data.
fingerprint-pro-server-api-php-sdk/src/Api/FingerprintApi.php
Lines 137 to 139 in ee54f5b
$response = $e->getResponseObject(); | |
$responseBody = $response->getBody()->getContents(); | |
$response->getBody()->rewind(); |
Let's move this part of code above the
switch
statement, so that it won't be repeated for every case
This applies to all error status codes.
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.
Almost there @Orkuncakilkaya, thanks for these changes so far!
Pls also check ObjectSerializer
and remove unused methods. I see that there are at least couple of them that are not used at all, such as toFormValue
and toHeaderValue
I made the changes based on your comments. Please resolve all previous unresolved comments if you think they are resolved |
There is one more thing that you need to fix.
What you need to do:
|
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.
LGTM, I have just one question 🙂
This PR will create a major release 🚀5.0.0 (2024-08-21)⚠ BREAKING CHANGES
Features
Bug Fixes
|
No description provided.