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

[WIP] MoyaProvider variant without generic parameter #910

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

manas-chaudhari
Copy link
Contributor

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.

@MoyaBot
Copy link

MoyaBot commented Jan 9, 2017

1 Warning
⚠️ PR is classed as Work in Progress

Generated by 🚫 danger

@codecov-io
Copy link

codecov-io commented Jan 9, 2017

Current coverage is 73.72% (diff: 100%)

Merging #910 into master will not change coverage

@@             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          

Powered by Codecov. Last update 9c0a41a...d4830f3

Copy link
Member

@ashfurrow ashfurrow left a 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.

@AndrewSB AndrewSB requested a review from scottrhoyt January 10, 2017 23:51
Copy link
Contributor

@scottrhoyt scottrhoyt left a 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 TargetTypes 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 ?

@manas-chaudhari
Copy link
Contributor Author

My opinion is that if the end-user prefers strong types over grouping, he should have that choice.

Earlier, I was expecting MultiMoyaProvider implementation to get complicated, hence I wanted this to be included in the core.
However, I tried to implement this and it has turned out to be very simple. Thus, with respect to implementation, I am fine with building it as an extension.

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.

@ashfurrow
Copy link
Member

The key deliverable here is the documentation.

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!

@ashfurrow ashfurrow merged commit 2f96939 into Moya:master Jan 13, 2017
@ashfurrow
Copy link
Member

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.

@AndrewSB
Copy link
Member

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 😄

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.

None yet

6 participants