Skip to content
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

[0.x] Adds Moderations::create typed response #5

Conversation

gehrisandro
Copy link
Collaborator

There we go.

see #4

@nunomaduro
Copy link
Contributor

nunomaduro commented Sep 30, 2022

@gehrisandro It's possible to focus this pull request in only on typed responses, and on a single resource method (Moderations::create)? So we can define their structure together, and have a good foundation for the remaining methods.

@nunomaduro nunomaduro marked this pull request as draft September 30, 2022 11:08
@nunomaduro nunomaduro changed the title POC: strong typed requests and responses [0.x] Adds Moderations::create typed response Sep 30, 2022
@gehrisandro gehrisandro force-pushed the poc-strong-typed-requests-and-responses branch from 0f7458b to a4c02b1 Compare September 30, 2022 19:44
@gehrisandro
Copy link
Collaborator Author

@nunomaduro I narrowed the pull request down to focus only on the typed response as you suggested.

In my opinion the typed response is very easy to understand and use.
But still there are some points I wasn't sure what the best approach would be:

  • Is it a good idea to rearrange the data returned from the API or should the structure represented in exactly the same way? For the moderation endpoint, I rearranged the resulting categories data from two arrays having the same keys to a single array with category objects, which feels for me more natural.

  • The response objects are namespaced under "DataObjects" and not just simple "Responses". In my opinion it expresses a bit better, that it isn't a 100% exact copy of what the API returned. (only if you agree to rearrange data as mentioned above)

  • Naming of the nested data objects: Moderation\ModerationCategory vs. Moderation\Category
    Probably it's an endless discussion which approach is better. In my opinion most important is to be consistent within the project. What do you prefer?

  • To create the data objects from the array I've created dedicated factory classes. Maybe it's a bit over engineered but it keeps the API call method every clean as it can simply call the factory with the data returned from the API.

  • Last but not least. I had to add a @phpstan-ignore-line in ModerationFactory::make() as I wasn't able to find a proper type declaration which doesn't lead to an error.

I'm looking forward to hearing your opinion.

src/Contracts/DataObject.php Outdated Show resolved Hide resolved
src/Contracts/DataObject.php Outdated Show resolved Hide resolved
src/DataObjects/Moderation/Moderation.php Outdated Show resolved Hide resolved
src/DataObjects/Moderation/Moderation.php Outdated Show resolved Hide resolved
src/DataObjects/Moderation/ModerationCategory.php Outdated Show resolved Hide resolved
src/DataObjects/Moderation/ModerationCategory.php Outdated Show resolved Hide resolved
@nunomaduro
Copy link
Contributor

nunomaduro commented Oct 1, 2022

@gehrisandro Left a few comments - regarding the way I would probably structure this.

@gehrisandro
Copy link
Collaborator Author

Thank you for our feedback.

I renamed classes / namespaces in the way to suggested, updated and cleaned update the tests.

@nunomaduro nunomaduro marked this pull request as ready for review October 3, 2022 21:07
@nunomaduro nunomaduro merged commit 1b0018f into openai-php:main Oct 3, 2022
@nunomaduro
Copy link
Contributor

Adjusting things locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants