-
Notifications
You must be signed in to change notification settings - Fork 6k
Support a true "java8" option #5864
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
Conversation
The "dateLibrary" option for java, sadly, sets a mustache value "java8". This change updates this so that "java" in the mustache libraries means what it should mean - use all java8 classes. In this case, there's no need for the third party Base64 library as java8's JDK has this built in. In my view, the "dateLibrary" should be deprecated but that should be a separate PR.
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.
Hmm, maybe also put the false value in the other case? (Or just additionalProperties.put("java8", java8Mode.toString());)
By the way, why are you using "java8" here and not JAVA8_MODE?
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 - it was used below. I'll change it.
You indicated in the PR comment that you updated the samples, but I don't see any changed samples. Are there no changes?
You indicated in the PR comment that you updated the samples
I ran the script - maybe I did something wrong? I'll check.
Can you check if it's not already fixed on branch 2.3.0 ? A jsr310 flag was introduced to distinguish dates and jdk.
@cbornet I checked branch 2.3.0 and I see threetenbp but that's still only applies to the dateLibrary. This PR is focused on using Java8 classes everywhere. In this instance, there's no need for the 3rd party Base64 library as that's builtin to Java 8 now.
This PR also "takes ownership" of the java8 mustache variable. IMO that should not have been used to denote which dateLibrary you want to use.
See https://github.com/swagger-api/swagger-codegen/blob/2.3.0/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/AbstractJavaCodegen.java#L382 : there is a jsr310 flag on 2.3.0 that is used specifically for JSR310 dates.
@cbornet I guess I'm not conveying the point of this PR. This is not about the date library. This is for Java8 classes in general. Base64 is part of the Java 8 JDK now. There is no longer any need to import a third party library for this purpose.
@Randgalt OK. Then I don't think we need yet another option (we already have too many) there should be only "java8" option for which we can use Base64
@cbornet this not a new option. This PR takes the "java8" mustache variable that is set by dateLibrary and uses it to mean what it implies: use java8 classes where needed. The only thing new is a CLI option to set "java8" - this is needed because it no longer makes sense to have "java8" set by dateLibrary (that part should be deprecated).
@Randgalt oh right, I misread your PR 😄 !
Then LGTM
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.
This duplication looks wrong. I guess you wanted to check for JAVA_8?
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.
Doh - copy/paste error. I'll fix it.
Second time. Not sure what to do:
[ERR!] GitHub API: GitHub rate limit reached.
To increase the limit use GitHub authentication.
See: https://github.com/DefinitelyTyped/tsd#tsdrc
@Randgalt I've restarted the Travis job. If it's still failing, I'll test locally.
ping
Test results after sample update are good: https://circleci.com/gh/swagger-api/swagger-codegen/518
@Randgalt PR merged into master. Thanks for your contribution.
I wonder if you can make the same enhancement to 2.3.0 branch as I got some merge conflicts syncing master into 2.3.0.
@wing328 sure
Uh oh!
There was an error while loading. Please reload this page.
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)2.3.0branch for breaking (non-backward compatible) changes.Description of the PR
The "dateLibrary" option for java, sadly, sets a mustache value "java8". This change updates this so that "java8" in the mustache libraries means what it should mean - use all java8 classes. In this case, there's no need for the third party Base64 library as java8's JDK has this built in. In my view, the "dateLibrary" value "java8" should be deaprecated but that should be a separate PR.