-
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
Feature/javaPlayWithAsynchronousControllers #7705
Feature/javaPlayWithAsynchronousControllers #7705
Conversation
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.
Please see my comments. Did you make sure to generate all the other samples for Play Framework? Since we don't see any change here, it's difficult to say if it's because there is no changes or if you forgot to run them.
Anyway, thanks for this very nice PR!
throw new IllegalArgumentException("'body' parameter is required"); | ||
} | ||
|
||
// controllerOnly false |
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.
Why is this there?
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.
Good catch, I miss to regenerate samples after removing that testing comments 😅
apiKey = null; | ||
} | ||
|
||
// controllerOnly false |
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.
same question
} | ||
} | ||
|
||
// controllerOnly false |
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.
same question
} | ||
} | ||
|
||
// controllerOnly false |
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.
etc...
Sorry, I had forgotten to regenerate the samples for the other play framework scenarios, It's done now |
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.
Please make sure your changes don't modify the current samples since it should be 100% independent.
@@ -17,6 +17,7 @@ | |||
import swagger.SwaggerUtils; | |||
import com.fasterxml.jackson.core.type.TypeReference; | |||
|
|||
|
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.
There is a lot of new unnecessary blank line. Can you change the mustache to prevent that?
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.
In the best case scenario, your changes would not create anything new in the old samples
Client obj = imp.testSpecialTags(body); | ||
if (configuration.getBoolean("useOutputBeanValidation")) { | ||
SwaggerUtils.validate(obj); | ||
} | ||
JsonNode result = mapper.valueToTree(obj); | ||
return ok(result); | ||
JsonNode result = mapper.valueToTree(obj); |
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.
here the tabulation is not good anymore
@@ -13,7 +13,7 @@ | |||
@Override | |||
public Client testSpecialTags(Client body) throws Exception { | |||
//Do your magic!!! | |||
return new Client(); | |||
|
|||
return new Client(); |
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.
tabulation not good anymore
@@ -11,6 +11,6 @@ | |||
|
|||
@SuppressWarnings("RedundantThrows") | |||
public interface AnotherFakeApiControllerImpInterface { | |||
Client testSpecialTags(Client body) throws Exception; | |||
Client testSpecialTags(Client body) throws Exception; |
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.
missing a space
Boolean obj = imp.fakeOuterBooleanSerialize(body); | ||
JsonNode result = mapper.valueToTree(obj); | ||
return ok(result); | ||
JsonNode result = mapper.valueToTree(obj); |
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.
bad tabulation
OuterComposite obj = imp.fakeOuterCompositeSerialize(body); | ||
if (configuration.getBoolean("useOutputBeanValidation")) { | ||
SwaggerUtils.validate(obj); | ||
} | ||
JsonNode result = mapper.valueToTree(obj); | ||
return ok(result); | ||
JsonNode result = mapper.valueToTree(obj); |
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 could continue on and on but like I said earlier, just make sure there is no changes in the previous samples and I will approve this PR. If you have trouble using the mustache file (it can be tricky), let me know exactly where you are struggling and I will try to help.
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.
there is an easy way to fix the tabulation issue without having to add each one of them manually? 😰
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 is generated, so if you fix the mustache file, it will be fix everywhere. Never edit manually the files in the "sample" folder.
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.
No, I meant in the mustache file
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 you only need to change it at one place. I don't have time right now to check it out but I'm sure you'll figure it out by trial and error :) If you are completely clueless, I could check it next week.
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.
ok I fixed the other scenarios, but the async one still lacks tabulations 😕
@@ -15,22 +15,22 @@ | |||
|
|||
@SuppressWarnings("RedundantThrows") | |||
public interface FakeApiControllerImpInterface { | |||
Boolean fakeOuterBooleanSerialize(Boolean body) throws Exception; | |||
Boolean fakeOuterBooleanSerialize(Boolean body) throws Exception; |
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.
all missing a space
@@ -749,8 +749,8 @@ | |||
"type" : "array", | |||
"items" : { | |||
"type" : "string", | |||
"default" : "$", | |||
"enum" : [ ">", "$" ] | |||
"enum" : [ ">", "$" ], |
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.
theses changes could be ignored, they are caused by the core that doesn't do some sorting and because of this, the order is totally random.
@ignaciomolina Nice job. I will take some time next week to help you fix the problem with the tabulation in the async sample. But for the rest, this look promising! |
@ignaciomolina Sorry about the delay, I'm very busy right now. Will try to check it somewhere today or tomorrow but can't be sure. Maybe beginning of next week. Sorry about that :) |
@ignaciomolina I'm checking this tomorrow morning! I didn't forget you! :) |
@ignaciomolina newApi.mustache
newApiController.mustache
|
Ok, I already add the changes and regenerate the samples, I am not fun of duplicating the validation block just to add tabulation though |
|
@@ -215,21 +226,50 @@ public class {{classname}}Controller extends Controller { | |||
SwaggerUtils.validate(obj); | |||
{{/returnContainer}} | |||
} | |||
{{/supportAsync}} | |||
{{#supportAsync}} |
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.
@wing328 I helped @ignaciomolina with his PR by suggesting this solution. To have good tabulation, we need to duplicate stuff in the mustache file (look here for the bean validation).
Any idea to do it more cleanly? Otherwise, this PR looks good to me!
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.
@JFCote agreed with you that some files or code (refactored into mustache partial) can be reused between different generators. I've some ideas but would require moving files around (which is not an elegant solution).
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.
@wing328 I'm not talking about moving stuff between generators, I'm talking about duplicated stuff in this mustache file. The line that is highlighted. There is the version with supportAsync
without tabulation and the version without supportAsync
that doesn't have any tabulation. The code is duplicated.
I was wondering if there was a better way to do that?
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.
@JFCote ah ok. Just went through the mustache file you provided.
What about creating another file newApiControllerAsync.mustache
instead of using {{#supportAsync}} .. {{/supportAsync}}
in newApiController.mustache
? The drawback is that we must remember applying the same enhancement/bug fixes to both files.
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.
That's even worse than duplicate a chunk of code xD
Maybe the best solution was the first, to just add {{#supportAsync}} {{/supportAsync}}
in front each line of the block 😓
By the way @wing328 , this is ok for merging I think |
PR checklist
./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). Windows batch files can be found in.\bin\windows\
.3.0.0
branch for changes related to OpenAPI spec 3.0. Default:master
.@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @tzimisce012
Description of the PR
CompletionStage
interface