-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[WIP] MoyaProvider variant without generic parameter #910
[WIP] MoyaProvider variant without generic parameter #910
Conversation
Generated by 🚫 danger |
Current coverage is 73.72% (diff: 100%)@@ master #910 diff @@
==========================================
Files 22 22
Lines 723 723
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 533 533
Misses 190 190
Partials 0 0
|
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.
Very cool! Thanks for adding this, I think it'll help a lot of users who aren't using the RAC/Rx providers for deserialization.
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.
I think this is a neat idea. However, as you mention in the documentation, it is somewhat limited to TargetType
s that only return one kind of object.
This encourages the organization of API information by return type instead of by use case. For example, it is quite common for GET /users/
to return a [User]
while GET /users/<id>
to return a User
. Thematically it makes sense for these to be in the same TargetType
, but they wouldn't be able to using this method. I think this breaks with some of the pattern promoted by Moya.
Also, because different JSON deserialization libraries use different method signatures for decoding JSON, it would require additional input for the user to write the conformance to SomeJSONDecodableProtocolConformance
for their chosen deserialization library.
This is a cool concept and definitely executed to the extent that Swift allows right now. But all this leads me to think this might be a much better candidate for a Moya community extension than addition to the core library. That way you could also include the conformance to one or more JSON deserialization libraries. What do you think @manas-chaudhari ?
My opinion is that if the end-user prefers strong types over grouping, he should have that choice. Earlier, I was expecting class MultiMoyaProvider: MoyaProvider<MultiTarget> { } The key deliverable here is the documentation. My only concern now is whether an extension which contains only documentation makes sense. |
Yup, this PR is only docs, which can only really help. I'm going to merge and we can refine later if we add first-class support into the library. Thanks again! |
Thanks a lot for contributing to Moya! I've invited you to join the Moya GitHub organization – no pressure to accept! If you'd like more information on what that means, check out our contributor guidelines and feel free to reach out with any questions. |
Cool! I'm happy the community saw value in this. @manas-chaudhari, we'll look for your next PR which implements the documentation we've added 😄 |
This is based on #830. I have updated only the docs to provide a usecase for why this is required.
I strongly think that this will enable end-users to extend Moya's functionality in different ways which are not possible right now.
Would love to have feedback on whether this makes sense.
I am not sure where exactly this documentation belongs. Fundamentally, this feature provides a MoyaProvider that can be used with different targets. Hence, I added it to
MultiTarget
.I would like to start working on the implementation as soon as this requirement is validated.