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

[Java] Implemented support for Play! 2.5.x (java8) for retrofit2 client #6007

Merged
merged 8 commits into from
Aug 31, 2017

Conversation

lukoyanov
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Support for Play! 2.5.x (java8) for retrofit2 client

@cbornet
Copy link
Contributor

cbornet commented Jul 7, 2017

Are we going to pull a new option for each new version of Play ?!

@wing328
Copy link
Contributor

wing328 commented Jul 8, 2017

@lukoyanov I agree with @cbornet that we should avoid introducing another option for this case.

For TypeScript Angular2, we recently introduced the option ngVersion so that developers can choose TS Angular 2x or 4x based on their use case.

For your case, I would suggest renaming the option to usePlayWS and make the change (this PR) to 2.3.0 as it's a breaking change.

If users need to use Play! (2.4.x), they can use the master.

@lukoyanov
Copy link
Contributor Author

lukoyanov commented Jul 11, 2017

@cbornet @wing328 ok, i'll take a look at TS Angular 2x implementation and update this PR.

@wing328
Copy link
Contributor

wing328 commented Jul 11, 2017

@lukoyanov if you need any help, just let me know.

implemented version switch via playVersion config option
@lukoyanov
Copy link
Contributor Author

lukoyanov commented Aug 12, 2017

I've finished the implementation and improved several things along the way (backport to play24 in #6302). Now Play! support can be enabled via usePlayWS config option, and specific version can be set via playVersion (play24, play25 is supported). @wing328 @cbornet

@lukoyanov
Copy link
Contributor Author

@wing328 Should I reopen this PR against 3.0 branch?

@wing328 wing328 reopened this Aug 15, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 15, 2017

@lukoyanov sorry I incorrectly closed it. Please still target current master for the change.

3.0.0 is mainly for supporting OpenAPI spec 3.0.

@wing328 wing328 modified the milestones: v2.3.0, Future Aug 15, 2017
@lukoyanov
Copy link
Contributor Author

lukoyanov commented Aug 15, 2017

@wing328 Fixed issues after merge.

@lukoyanov
Copy link
Contributor Author

@wing328 @cbornet Is there something else I should fix/implement here?

@wing328
Copy link
Contributor

wing328 commented Aug 28, 2017

@lukoyanov Let me review tomorrow and merge accordingly (sorry too busy these days...)

@wing328
Copy link
Contributor

wing328 commented Aug 30, 2017

@lukoyanov I ran ./bin/java-petstore-retrofit2-play25.sh to udpate the petstore sample but got errors running mvn test in samples/client/petstore/java/retrofit2-play25.

Please take a look when you've time.

@lukoyanov
Copy link
Contributor Author

@wing328 sorry, fixed it.

@wing328 wing328 merged commit a2c4f0b into swagger-api:master Aug 31, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 31, 2017

@lukoyanov np. I performed some tests and the result is good.

Thanks for the enhancement.

@lukoyanov lukoyanov deleted the retrofit2-playws25 branch August 31, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants