-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Issue 5497: Support the use of tags in the delegated Spring Kotlin generator. #5499
Issue 5497: Support the use of tags in the delegated Spring Kotlin generator. #5499
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.
Sorry for the late review. I'm pretty backed up lately.
I just had a small question in the PR, but other than that it looks good. I'd be happy to update and resolve any conflicts before merge if you'd like.
@@ -440,7 +451,7 @@ public void processOpts() { | |||
|
|||
@Override | |||
public void addOperationToGroup(String tag, String resourcePath, Operation operation, CodegenOperation co, Map<String, List<CodegenOperation>> operations) { | |||
if (library.equals(SPRING_BOOT) && this.delegatePattern) { | |||
if (library.equals(SPRING_BOOT) && !useTags) { |
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 changes previous behavior for library.equals(SPRING_BOOT) && this.delegatePattern
and would be a breaking change. Was this intentional? If so, can you speak toward the intention? To me it looks like this.delegatePattern
was accidentally removed.
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.
Thanks for the comment Jim. The replacement of this.delegatePattern with useTags was intentional. I'm currently using the delegate pattern with tags in my Java project and this change was needed to get the Kotlin generator to produce equivalent code to the Java one. In fact if you look at SpringCodegen.java you can see it does something very similar
if ((library.equals(SPRING_BOOT) || library.equals(SPRING_MVC_LIBRARY)) && !useTags) {
I understand that this is a breaking change but I'm not sure we can support tags properly while keeping backwards compatibility.
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.
Fair enough. I'll mark this PR as a breaking change. Thanks for the explanation.
This contains a potential breaking change in that the condition:
Was changed to:
This was to match the Java spring generator and appears to have no major impact on the delegate pattern with or without |
* master: (586 commits) Fix typescript generator for parameter collectionFormat for pipes ssv (#6553) [C++][Pistache] Catch HttpError from user-provided handler (#6520) remove scala related profile from the pom (#6554) move ruby tests to travis (#6555) [Java][jersey2] fix cast error for default value in DateTimeOffset object (#6547) [Swift] fix GET request with array parameter (#6549) [kotlin][spring] Fix ApiUtil compilation (#6084) update python samples [Python] Fixed docstrings in api.mustache (#6391) [BUG][python] Support named arrays (#6493) [Go] whitelist AdditionalProperties in the field name (#6543) [kotlin][client] remove tabs usage (#6526) [PS] automatically derive discriminator mapping for oneOf/anyOf (#6542) [C++][Ue4] various bus fixes (#6539) Fix incorrect npx command (#6537) update pester to 5.x (#6536) comment out openapi3 java jersey2-java8 tests add additional properties support to powershell client generator (#6528) [Go][Experimental] Support additionalProperties (#6525) #5476 [kotlin] [spring] fix swagger and spring annotation for defaultValue (#6101) ...
Issue #5497
Followed the example of the Java Spring implementation.
There are a lot of changed files in the generated PetStore. It appears the classes were slightly different before. Compared to the Java output and it matches the behavior of this PR.
Added a test in KotlinSpringServerCodegenTest (delegateWithTags)
PR checklist
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.@jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11)