Skip to content

[Kotlin][Client] Add option to make all api method return a nullable model #4422

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

Merged
merged 4 commits into from
Nov 13, 2019

Conversation

AlexanderEggers
Copy link
Contributor

@AlexanderEggers AlexanderEggers commented Nov 8, 2019

Closes #4418

  • added new additional-parameter nullableReturnType
    • converts the api related methods from non-null to nullable
    • change the class member data of ApiInfrastructureResponse.Success to a nullable type
  • added new kotlin-client sample to show the nullable changes
  • added new shell and bash script files

It probably makes sense to merge #4419 first. I will take care of any merge conflicts.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./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).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@auto-labeler auto-labeler bot added the WIP Work in Progress label Nov 8, 2019
@auto-labeler
Copy link

auto-labeler bot commented Nov 8, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@AlexanderEggers AlexanderEggers changed the title [Kotlin-Client][WIP] Add option to make all api method return a nullable model [Kotlin][Client][WIP] Add option to make all api method return a nullable model Nov 8, 2019
@AlexanderEggers AlexanderEggers marked this pull request as ready for review November 8, 2019 14:18
@AlexanderEggers AlexanderEggers changed the title [Kotlin][Client][WIP] Add option to make all api method return a nullable model [Kotlin][Client] Add option to make all api method return a nullable model Nov 8, 2019
@AlexanderEggers AlexanderEggers force-pushed the kotlin-nullable branch 2 times, most recently from 617b5a4 to 7920c09 Compare November 8, 2019 14:32
@AlexanderEggers
Copy link
Contributor Author

PR is ready for review.

@jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10)

@AlexanderEggers AlexanderEggers force-pushed the kotlin-nullable branch 4 times, most recently from 89bbec8 to a010d25 Compare November 11, 2019 08:46
@AlexanderEggers
Copy link
Contributor Author

@wing328 Since I dragged in your PR, the circleci step is failing. Do you have an idea for how to fix that? Besides that, the PR is ready for review. I fixed the merge conflicts that were caused by your PR.

Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great addition! Just had a few questions.

Should we also maybe consider adding a default of false to the generator, so it's clear in code that this is an option? I eventually want to make additionalProperties sorted by embedded templates to be discoverable.

@AlexanderEggers
Copy link
Contributor Author

Thanks for your feedback. 👍

Should we also maybe consider adding a default of false to the generator, so it's clear in code that this is an option? I eventually want to make additionalProperties sorted by embedded templates to be discoverable.

Do you mean adding the option to this readme >> https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/kotlin.md

@jimschubert
Copy link
Member

@Mordag regarding the default in the generator, I was referring to something like this:

if (additionalProperties.containsKey(CodegenConstants.SOURCE_FOLDER)) {

However, based on the other discussions, it looks like there won't be other changes so I can just pick up the additionalProperties thing later.

My only remaining question on the PR is related to the mediaType guard.

Let me know if you'd like help looking into the CI error. You mentioned it failing after pulling in changes, but I don't see an additional commit (so I didn't know where to begin). It seems from looking at the logs that it failed on Kotlin generation. It could be as simple as missing the executable flag on bin/kotlin-client-nullable.sh

@AlexanderEggers
Copy link
Contributor Author

@jimschubert I removed the mediaType for now and updated the samples. If all tests are green, the PR should be ready to 🚢.

@AlexanderEggers
Copy link
Contributor Author

@jimschubert The circleci is going to fail with the same error again. Can you help me try to figure out the root problem?

Mordag and others added 3 commits November 12, 2019 23:31
I've added new .bat files for the kotlin-client to alllow windows-devs to re-generate required samples via windows-shell (for CI).
In this commit I have also made some changes to the kotlin-client-all.bat to make this file a bit more robust regarding different folder structures.
@jimschubert
Copy link
Member

@Mordag I've opened AlexanderEggers#1 against your branch. This should resolve the CircleCI issue.

I didn't merge and fix up in master because your PR is still marked WIP, and GitHub makes it look like you did a force push after you gave thumbs up. If you merge the PR and remove WIP, then I think your changes look good and can be merged once CI is green.

Looks like the Shippable failure is just Shippable being flaky on container initialization, it was green on the previous commit, prior to the force push (which removes links to those checks)… see Shippable runs/12015

@AlexanderEggers
Copy link
Contributor Author

AlexanderEggers commented Nov 13, 2019

@jimschubert Thanks for helping me resolving the issue with the circleci. 👍

I didn't merge and fix up in master because your PR is still marked WIP, and GitHub makes it look like you did a force push after you gave thumbs up. If you merge the PR and remove WIP, then I think your changes look good and can be merged once CI is green.

How do I remove the WIP tag? It doesn't seem that I have the needed permissions for that. The force push is only related to a rebase to master.

@jimschubert jimschubert added Client: Kotlin and removed WIP Work in Progress labels Nov 13, 2019
@jimschubert
Copy link
Member

No problem! I've gone ahead and removed the WIP. I see now that it was added by auto-labeler and you have since changed the title (hadn't noticed that earlier).

@jimschubert jimschubert added this to the 4.2.1 milestone Nov 13, 2019
@jimschubert jimschubert merged commit 34c715e into OpenAPITools:master Nov 13, 2019
@AlexanderEggers AlexanderEggers deleted the kotlin-nullable branch November 13, 2019 03:50
@wing328
Copy link
Member

wing328 commented Nov 15, 2019

@Mordag thanks for the PR, which has been included in the v4.2.1 release: https://twitter.com/oas_generator/status/1195339336922759168

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.

[REQ][Kotlin-Client] Add nullable return type for api classes
4 participants