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

Issue 1766 Modified mustache files for Go to support nullable in the … #1869

Merged
merged 14 commits into from
Feb 15, 2019

Conversation

alex-korobko
Copy link
Contributor

…spec v3.0+; Updated model files running .sh scripts for Go.

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.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

Updated mustache files to enable the nullable support (#1766)
Ran all ./bin/go*.sh files to update the models;
Not sure though how to test the changes though/ The integration test (as it recommended in the guide) classes for the Golang are using specs from the ver 2.0, so it seems like there is no reason to use them. Should we try to make new classes for that? Do you have any guide for that?

@antihax @bvwells @grokify @kemokemo

…spec v3.0+; Updated model files running .sh scripts for Go.
@@ -19,6 +19,6 @@ const (
type {{classname}} struct {
{{#vars}}{{#description}}
// {{{description}}}{{/description}}
{{name}} {{^isEnum}}{{^isPrimitiveType}}{{^isContainer}}{{^isDateTime}}*{{/isDateTime}}{{/isContainer}}{{/isPrimitiveType}}{{/isEnum}}{{{dataType}}} `json:"{{baseName}}{{^required}},omitempty{{/required}}"{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the change that I'd like to discuss. In a previous version relative model objects were generated as classes. As we have nullable support now, I don't think we need to keep this implicit behavior. I'd rather expect to have the pointer defined as it should be - using the nullable property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your PR! 😃
I agree with you.

@@ -19,6 +19,6 @@ const (
type {{classname}} struct {
{{#vars}}{{#description}}
// {{{description}}}{{/description}}
{{name}} {{^isEnum}}{{^isPrimitiveType}}{{^isContainer}}{{^isDateTime}}*{{/isDateTime}}{{/isContainer}}{{/isPrimitiveType}}{{/isEnum}}{{{dataType}}} `json:"{{baseName}}{{^required}},omitempty{{/required}}"{{#vendorExtensions.x-go-custom-tag}} {{{.}}}{{/vendorExtensions.x-go-custom-tag}}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the change that I'd like to discuss. In a previous version relative model objects were generated as classes. As we have nullable support now, I don't think we need to keep this implicit behavior. I'd rather expect to have the pointer defined as it should be - using the nullable property.

@kemokemo
Copy link
Contributor

I have made a brief review.
There is something I want to check one point so please give me some time.

@ackintosh
Copy link
Contributor

ackintosh commented Jan 11, 2019

@alex-korobko Thanks for the PR! 👍


TO: @OpenAPITools/generator-core-team

#1869 (comment)

Updated mustache files to enable the nullable support (#1766)
Ran all ./bin/go*.sh files to update the models;
Not sure though how to test the changes though

I think we need to add nullable to "3_0/petstore-with-fake-endpoints-models-for-testing.yaml" to make sure that the output is generated as we expected. do you have any concerns?

@alex-korobko
Copy link
Contributor Author

alex-korobko commented Jan 14, 2019

@alex-korobko Thanks for the PR! 👍

TO: @OpenAPITools/generator-core-team

#1869 (comment)

Updated mustache files to enable the nullable support (#1766)
Ran all ./bin/go*.sh files to update the models;
Not sure though how to test the changes though

I think we need to add nullable to "3_0/petstore-with-fake-endpoints-models-for-testing.yaml" to make sure that the output is generated as we expected. do you have any concerns?

Not sure if it could help, the 3_0/petstore-with-fake-endpoints-models-for-testing.yaml file is not used in go test scripts yet, so we should probably need more changes in order to get nullable testable for Golang model generation.

@ackintosh
Copy link
Contributor

Sorry I didn't explain it enough. 💦
I think we need to add a shell script under ./bin/*openapi3* like bin/openapi3/php-petstore.sh before adding nullable to the yaml file.
(The script uses petstore-with-fake-endpoints-models-for-testing.yaml)

@alex-korobko
Copy link
Contributor Author

alex-korobko commented Jan 17, 2019

Sorry I didn't explain it enough. 💦
I think we need to add a shell script under ./bin/*openapi3* like bin/openapi3/php-petstore.sh before adding nullable to the yaml file.
(The script uses petstore-with-fake-endpoints-models-for-testing.yaml)

Got it, thanks! So who should create that Shell script to test v3 spec features for Go? Did you recommend me to make the script in this PR or guys from the team are adding such scripts using some other internal workflow?

@kemokemo
Copy link
Contributor

Sorry for the late review.
I have completed checking for the code you had already committed. They looks good. 👍
There is still content to discuss, but I will report it. 👀

@kemokemo
Copy link
Contributor

kemokemo commented Feb 2, 2019

To: @alex-korobko @ackintosh

Hi!

I had added the nullable property to the petstore-with-fake-endpoints-models-for-testing.yaml and created go-petstore.sh.
I could not push to alex-korobko/openapi-generator repository so I saved it to my gist. Sorry.. 🙇

Can you confirm the content? Also, if it seems good, please apply it to this repository.
Then I want you to run the script and update the sample.

@ackintosh
Copy link
Contributor

@kemokemo Thanks for your good job! I'm working on that applying your changes to this PR. 😉

@ackintosh
Copy link
Contributor

ackintosh commented Feb 5, 2019

The updates from @kemokemo has been pushed to this PR. Please take a look if the sample (which is generated from a spec that contains nullable) is generated as you guys expected. @kemokemo @alex-korobko

@alex-korobko
Copy link
Contributor Author

Thanks a lot for your help! Let me check the updates.

@ackintosh
Copy link
Contributor

FYI: The updates before has generated only "go client",
so in order to generate "go gin server" and "go server" samples I've added bin/openapi3/go-gin-petstore-server.sh and updated bin/openapi3/go-petstore-server.sh.

@ackintosh
Copy link
Contributor

The samples added by this PR:

It looks good for me as the parameter defined as nullable is generated as pointer like below.

NullableMessage *string `json:"NullableMessage,omitempty"`

@ackintosh ackintosh added this to the 4.0.0 milestone Feb 9, 2019
@kemokemo
Copy link
Contributor

@ackintosh Thank you so much for your merging! 😄
I had checked the sample code generated using petstore-with-fake-endpoints-models-for-testing.yaml and found a potential problem with go-server and go-gin-server.

issue

The import statement is duplicated.

plan to resolve above

 {{>partial_header}}
 package {{packageName}}
 {{#models}}{{#imports}}
-import ({{/imports}}{{#imports}}
-	"{{import}}"{{/imports}}{{#imports}}
-)
+{{#-first}}import ({{/-first}}
+    "{{import}}"
+{{#-last}}){{/-last}}
 {{/imports}}{{#model}}{{#isEnum}}{{#description}}// {{{classname}}} : {{{description}}}{{/description}}
 type {{{name}}} {{^format}}{{dataType}}{{/format}}{{#format}}{{{format}}}{{/format}}

I am sorry many times.
Could @ackintosh or @alex-korobko please apply the above contents to the following file? 🙇

@ackintosh
Copy link
Contributor

I'm working on the issue. 😉

@ackintosh
Copy link
Contributor

I've updated the templates, please have a look when you have time. ✨

@kemokemo
Copy link
Contributor

@ackintosh , thanks a lot for your support! 😉
It looks very nice. My review has been completed.

Everyone, please point out if you notice any problems.
Thank you!

@ackintosh ackintosh merged commit 42544b8 into OpenAPITools:master Feb 15, 2019
@ackintosh
Copy link
Contributor

The PR has been merged into master. Thanks for your contribution! @alex-korobko @kemokemo 👍 👍 👍

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
OpenAPITools#1869)

* Issue 1766 Modified mustache files for Go to support nullable in the spec v3.0+; Updated model files running .sh scripts for Go.

* Add "nullable" to fake yaml

* Add sample script for OAS3

* Fix output folder (openapi3)

* Run bin/openapi3/go-petstore.sh

* Update samples

* Update jaxrs-jersey

* Update python and php samples

* Add bin/openapi3/go-gin-petstore-server.sh

* Run bin/openapi3/go-gin-petstore-server.sh

* Update bin/openapi3/go-petstore-server.sh to generate "nullable" samples

* Run bin/openapi3/go-petstore-server.sh

* Fix duplicated `import`
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.

3 participants