Add mutable model option to kotlin generators #4115
Conversation
| {{>property}} {{{name}}} get() = _{{{name}}} ?: throw IllegalArgumentException("{{{name}}} is required") | ||
| {{#modelMutable}}set(value){ _{{{name}}} = value }{{/modelMutable}} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@Zomzog can you please resolve the merge conflicts when you've time? |
|
Tested locally and the result is good. |
|
@Zomzog thanks for the PR, which has been included in the v4.2.2 release: https://twitter.com/oas_generator/status/1201432648544972800 |
PR checklist
./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.shif 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.master,4.1.x,5.0.x. Default:master.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