Go struct members pointers #3241
Conversation
| @@ -35,9 +35,95 @@ type {{classname}} struct { | |||
| {{#description}} | |||
| // {{{description}}} | |||
| {{/description}} | |||
| {{name}} {{#isNullable}}*{{/isNullable}}{{{dataType}}} `json:"{{baseName}}{{^required}},omitempty{{/required}}"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}` | |||
| {{name}} *{{{dataType}}} `json:"{{baseName}},omitempty"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}` | |||
There was a problem hiding this comment.
what's the point of adding a field isExplicitNull to mark it explicity? as a convention of go, it's a nullable if it's a pointer, otoh, a non-pointer field means it's a non-optional field
There was a problem hiding this comment.
The point is to be able to have a field that can be sent with null as a value. The whole point of this PR is that it's very hard in Go to have fields that allow setting either a zero value (0, false, empty string) or a null explicitly. This PR allows for various combinations of all of these features thanks to isExplicitNull and the custom marshalling.
There was a problem hiding this comment.
I notice that {{#isNullable}} ... {{/isNullable}} has been removed. Does it mean model properties with or without nullable set to true are handled the same way?
There was a problem hiding this comment.
These are handled explicitly in the custom marshalling method, see https://github.com/OpenAPITools/openapi-generator/pull/3241/files#diff-6403a68f6b1039398715805f9043d0a1R95 and following lines.
|
|
||
| // Get{{name}}Ok returns a tuple with the {{name}} field if it's non-nil, zero value otherwise | ||
| // and a boolean to check if the value has been set. | ||
| func (o *{{classname}}) Get{{name}}Ok() ({{dataType}}, bool) { |
There was a problem hiding this comment.
the name doesn't really sonuds ok.. in favor of others e.g. Get{{name}}IfExists..
There was a problem hiding this comment.
I actually prefer Get{{name}}Ok, since it returns two values - the value of {{name}} and whether or not retrieving it was ok. With IfExists, I would expect the function to return only the value of {{name}} if it exists. YMMV.
| @@ -35,9 +35,95 @@ type {{classname}} struct { | |||
| {{#description}} | |||
| // {{{description}}} | |||
| {{/description}} | |||
| {{name}} {{#isNullable}}*{{/isNullable}}{{{dataType}}} `json:"{{baseName}}{{^required}},omitempty{{/required}}"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}` | |||
| {{name}} *{{{dataType}}} `json:"{{baseName}},omitempty"{{#withXml}} xml:"{{baseName}}{{#isXmlAttribute}},attr{{/isXmlAttribute}}"{{/withXml}}{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}` | |||
There was a problem hiding this comment.
also, it's breaking backward-compatibility. the field shouldn't twiddle between pointer and non-pointer at least
There was a problem hiding this comment.
Yeah, I've had a discussion about this with @wing328 and we'll probably create another go client generator to accept this change (and possibly other breaking changes) that would otherwise be unacceptable for 4.1.x and would have to wait for 5.0.0 - which would be a loooong time.
There was a problem hiding this comment.
@bkabrda For the new go generator, would you mind tagging it with "Experimental"
It would look like this in the new generator's constructor:
generatorMetadata = GeneratorMetadata.newBuilder(generatorMetadata)
.stability(Stability.EXPERIMENTAL)
.build();
This will allow us to tag/filter generators to clearly present which ones are considered stable to users.
| {{/isNullable}} | ||
| {{/vars}} | ||
|
|
||
| func (o {{classname}}) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
- this should belong to another pull, discriminating the change..
- the custom marshal has to be a opt-in extension..
There was a problem hiding this comment.
- As noted in one of the comments above, the custom marshalling is a core of this change without which the rest wouldn't work reasonably.
- Why?
|
As discussed, you will resubmit the change for |
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
@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09)
This is a newly opened PR #3113, since the old one failed rebasing to 4.1.x in the Github UI. See #3113 for previous discussion.