-
Notifications
You must be signed in to change notification settings - Fork 6k
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] add template for Retrofit and Gson #706
[WIP] add template for Retrofit and Gson #706
Conversation
Current version breaks test "testLoginUser()". While being declared able to return "application/json" and "application/xml", endpoint "/user/login" returns plain string. This behavior breaks Gson parser, and I still haven't figured out elegant solution for this case. To my mind, it can be server issue (really, this is strange to return structured data all the way but plain string in rare cases). |
@olensmar - feel like checking it out? |
@xirt4m Fixed in last commit, thank you |
@olensmar ping |
sorry - digging in! |
import java.util.*; | ||
import java.io.File; | ||
|
||
public class RetrofitClientCodegen extends DefaultCodegen implements CodegenConfig { |
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.
it seems to me you could derive from JavaClientCodegen instead and get rid of a lot of duplicate code!?
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.
Well, I took AndroidClientCodegen as reference, and it doesn't derive from JavaClientCodegen. Default constructor adds lot of stuff to additionalProperties and supportingFiles, so any change in java codegen can break its subclasses. Yes, I can just clear these lists before setting my own values, but it can break some stuff tat will be done in future versions of DefaultCodegen
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.
Then perhaps we should have an AbstractJavaCodegen that all these derive from or put that shared functionality in some kind of helper (i.e. inheritance vs composition). If you don't have time for either please let me know so I can open a corresponding ticket after your PR has been merged. Thank you!
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.
It's a good idea, I'll take a look at it in couple of minutes.
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.
great! let me know if I can help in any way...
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 doubt how to include additional properties like invokerPackage, groupId, etc.. I think it should be specified in each codegen separately, but I want to avoid copy-pasted solution.
Also, there are lot of methods that use class fields like sourceFolder which can't be overridden. I hope there'll not become an issue if someone wants to use structure different from "src/java/main/my/favorite/package", it's very rare case, and shouldn't be covered by codegen. Anyway, user can set this field in deriver constructor
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.
have a look at #805 - I think it provides the footwork for this?
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.
Yes, so it'll be better to wait for changes from #805 to be applied (or not), then to make a decision about generalisation java clients
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.
makes sense - thank you!
[WIP] add template for Retrofit and Gson
Very cool - thank you! |
@0legg any chance for upgrading to Retrofit 2? |
@cemo Will take a look at it slightly later (well, Retrofit 2 is still in beta, so I have enough time =) ) |
@0legg I have actually done internal implementation based on your work. However It should be polished. :) |
Java and Android templates holds a lot of boilerplate code and leads user to include heavyweight libraries. Moreover, Apache HttpClient is deprecated for Android and it's usage can lead user to unstable behavior.
Square Retrofit library allows to dramatically reduce source code size without growing app footprint. As a bonus, Gson is much lighter library than Jackson, while covers most of cases used in Swagger.
This PR can affect issues #686, #687, and definitely #465