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

Add mutable model option to kotlin generators #4115

Merged
merged 5 commits into from Nov 27, 2019
Merged

Conversation

@Zomzog
Copy link
Contributor

Zomzog commented Oct 9, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I've change vertX generator for generating value object by default and add an option for generating mutable models in all kotlin generators.

I keep the immutable by default because I think it's the way to go with Kotlin.

@jimschubert
@dr4ke616
@karismann
@Wooyme

Copy link
Member

jimschubert left a comment

Thanks for the PR. I just had a concern about the externalization of the keyword, wondering if property.mustache could be inlined into the other templates maybe?

{{>property}} {{{name}}} get() = _{{{name}}} ?: throw IllegalArgumentException("{{{name}}} is required")
{{#modelMutable}}set(value){ _{{{name}}} = value }{{/modelMutable}}
Comment on lines 39 to 40

This comment has been minimized.

Copy link
@jimschubert

jimschubert Oct 10, 2019

Member

I wasn't sure where else to add this comment, so I'm adding it only to this file but it applies to everywhere that {{>property}} is being used.

I'm wondering if we can remove property.mustache and just inline the conditionals? I know it's more code, but what is being templated in this file isn't the "property" but the keyword used to declare the property. While scanning through the PR, I thought "this is going to cause issues if someone mistakes this for a "property template" and wants to add a setter. These two lines represent my concern pretty closely...

By adding a new property.mustache file, we're effectively creating a contract for those who customize the templates. OpenAPI Generator allows users to customize only the templates they want to have modified, and falls back to all built-in templates not defined by the user. This could cause issues, for example, if we renamed property.mustache to property_keyword.mustache or something in the future in order to externalize the property declaration (the two lines I've commented here) to property.mustache. In this situation, it would result in custom templates having properties defined as only val or var and no actual property.

Aside from the hypothetical, it also means that we have {{#modelMutable}} in more than one template just to define a single property. That seem like a lot of maintenance overhead.

This comment has been minimized.

Copy link
@wing328

wing328 Nov 18, 2019

Member

By adding a new property.mustache file, we're effectively creating a contract for those who customize the templates.

We've done that by using mustache partial to be included in many other generators. I think we did rename the file 1 or 2 times but no one has complained so far.

This could cause issues

Agree it will cause issues in the worst case but users should immediately notice that when they regenerate the code.

This comment has been minimized.

Copy link
@jimschubert

jimschubert Nov 19, 2019

Member

Could we maybe rename to modelMutable.mustache? I think that would sufficiently address my concerns about future use cases, but it does mean that there's some oddity around the use of the line above, which is not addressed. From a user perspective, if I create a template for "property" or "modelMutable", I'd expect that applied everywhere. If we can't reuse a template cleanly, I still think it should be inlined (it's not reusable). Maybe this could be solved with a "mutableSetter" partial... but then we have to consider maintainability of jumping around so many partials.

This comment has been minimized.

Copy link
@Zomzog

Zomzog Nov 20, 2019

Author Contributor

Sorry for the delay in response.
I've renamed it to modelMutable.mustache for the moment. I've created a partial because I don't want to copy/paste it 4 times for spring. But I understand the argument of too many partials and if you really prefer I can inline everything.

@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Nov 19, 2019

@Zomzog can you please resolve the merge conflicts when you've time?

@Zomzog Zomzog force-pushed the Zomzog:3803 branch from 28bbbc6 to d1288ca Nov 20, 2019
@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Nov 27, 2019

Tested locally and the result is good.

@wing328 wing328 merged commit 2dc0220 into OpenAPITools:master Nov 27, 2019
5 checks passed
5 checks passed
Shippable Run 12309 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rasmusfaber added a commit to rasmusfaber/openapi-generator that referenced this pull request Nov 28, 2019
* Add mutable model option to kotlin generators

fix OpenAPITools#3803

* doc

* Change template name to modelMutable

* Samples
rasmusfaber added a commit to rasmusfaber/openapi-generator that referenced this pull request Nov 28, 2019
* Add mutable model option to kotlin generators

fix OpenAPITools#3803

* doc

* Change template name to modelMutable

* Samples
@wing328

This comment has been minimized.

Copy link
Member

wing328 commented Dec 2, 2019

@Zomzog thanks for the PR, which has been included in the v4.2.2 release: https://twitter.com/oas_generator/status/1201432648544972800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.