Sjhunt93
commented
2 years ago In its current implementation (the SDK as a whole) - logically it does not make sense for any classes to inherit from APIRequest, as it is simply a collection of static functions. Without member variables. Instead UsersManager should be 'composed' of the APIRequest class. i.e. user manager should have that an APIRequest object as a member variable.
Ideally each function within the API, i.e. script should call a APIRequest.post() and know how to form the json data being sent and then know how to deal with the response schema. The problem here is also we cannot change response schemas for API calls and the previous SDK versions will break (as the objects cannot be constructed client side) - not sure exactly how we can make this robust for future use
What is concerning/interesting about our SDK is the mixing of functional and OO programming. Ironically, we could probably do away with inheritance and base our design on mostly functional design patterns.
Dont get me wrong this is good work, but for v2 I would strongly argue for a redesign.
Lastly, since voice cloning is somewhat detached from our main API offering, should we not release this as a separate SDK? i.e. apiaudio-voice-cloning. @springcoil
Why not following the convention of previous resources?
@Sjhunt93 and I have been discussing changes to introduce in SDK and so the convention will be changed for v1.0.0 (talking to V2 API at that point).
Briefly:
Making inheritance correct. Resources inherit APIRequest, yet instead of overwriting methods, plenty of if conditions are thrashing the original methods. Anti-pattern.
There's lots of repeated code in APIRequest because of having separate methods per HTTP method, even though often only one line of code is different. And that's a consequence of decisions mentioned in point 1.
Most important. We'll be having resource objects representing the actual API-side entities and their states (as you can see in this PR) instead of returning dictionaries.
Benefits are such that we're increasing:
It just makes more sense.
Note
Please try to be critical towards the proposition presented in this PR, because once it's accepted it'll lay foundation to the rest VC resource and likely the incoming refactors for V2.