[go-experimental] Add model constructors to initialize non-container vars with defaults by bkabrda · Pull Request #5175 · OpenAPITools/openapi-generator

bkabrda · 2020-01-31T11:10:42Z

This PR adds a constructor to all models that initializes non-container vars with proper defaults. It might also be possible to do container vars in the future, but that is pretty tricky (and also not as widely used to my knowledge).

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.

@antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

jirikuncar · 2020-01-31T11:25:31Z

modules/openapi-generator/src/main/resources/go-experimental/model.mustache

can you add a docstring?

Any reason not to require the required Params in the input?

@jirikuncar thanks for pointing out the docs I'll fix that.

@platinummonkey I think the problem here is that it's not straightforward what to do with required params that have defaults set. Should these be required arguments for the constructor or not? I think that this uncertainty is why generated constructors in other similar languages don't have required params either. I'd like to stay consistent here, so that's why I chose this approach.

I pushed a new commit that adds both the docstring and also documentation in model_doc.

New<ClassName>(args....) vs NewDefault<ClassName>() maybe is more clear?

@platinummonkey I just realized one more reason that is problematic with the required properties:

Imagine that you have required properties A and B and code generated for that: NewFoo(A, B). Now a change happens that makes B optional. You regenerate the code and suddenly you get NewFoo(A) - so that's backwards incompatible. But the change you did in API is backwards compatible, so this breaks the promise of not breaking API of the generated client when a non-breaking API change happens.

I'm not actually sure that's a problem, if you need to introduce a var, this this method signature is also changed. The alternative is to list all variables, and if one becomes non-optional though or a new one is added then still the signature is changed. Seem moot given go's syntax.

The signature of this method will never change, as it never accepts any arguments. But I think I'm starting to like the solution proposed in your previous comment with New(required, args, ...) and NewWithDefaults(). Assuming we clearly document that the contract of the former will always change when the set of required arguments changes, it should be ok. I'll work on this.

samples/client/petstore/go-experimental/go-petstore/model_animal.go

jirikuncar · 2020-02-04T21:05:54Z

...-generator/src/main/java/org/openapitools/codegen/languages/GoClientExperimentalCodegen.java

bkabrda · 2020-02-05T08:30:23Z

For anyone reviewing, please hold merging this until #5150 is merged. Thanks!

…vars with defaults

…ate constructors right under all circumstances

bkabrda · 2020-02-11T08:26:47Z

Ok, rebased and ready to merge now.

bkabrda · 2020-02-11T12:54:49Z

FWIW, bitrise failed because of an unrelated error (at least that's how I understand it):

==> Installing maven
Error: An exception occurred within a child process:
  DownloadError: Failed to download resource "maven"
Download failed: Couldn't determine mirror, try again later.
Can't install formulas:  exit status 1
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | brew-install@0.10.2 (exit code: 1)                            | 2.1 min  |
+---+---------------------------------------------------------------+----------+
| Issue tracker: https://github.com/bitrise-steplib/steps-brew-install/issues  |
| Source: https://github.com/bitrise-steplib/steps-brew-install                |
+---+---------------------------------------------------------------+----------+

…vars with defaults (OpenAPITools#5175) * [go-experimental] Add model constructors to initialize non-container vars with defaults * Add docstring and extend model_doc to include the constructor * Make variable names in constructor follow Go naming guidelines * Provide 2 different constructurs as suggested + couple fixes to generate constructors right under all circumstances * Fix defaults for enums * Properly escape vars that have reserved names

bkabrda force-pushed the go-experimental-struct-constructor branch from a7d7efc to 9df0d30 Compare January 31, 2020 11:21

jirikuncar reviewed Jan 31, 2020

View reviewed changes

platinummonkey reviewed Jan 31, 2020

View reviewed changes

samples/client/petstore/go-experimental/go-petstore/model_animal.go Outdated Show resolved Hide resolved

bkabrda force-pushed the go-experimental-struct-constructor branch from d0da1f1 to d2d9dd4 Compare February 3, 2020 13:38

jirikuncar reviewed Feb 4, 2020

View reviewed changes

...-generator/src/main/java/org/openapitools/codegen/languages/GoClientExperimentalCodegen.java Outdated

Copy link

Contributor

jirikuncar Feb 4, 2020

Choose a reason for hiding this comment

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

👍

jimschubert added the Client: Go label Feb 5, 2020

jimschubert added this to the 4.3.0 milestone Feb 5, 2020

bkabrda mentioned this pull request Feb 5, 2020

[go-experimental] Add oneOf support #5150

Merged

5 tasks

Slavek Kabrda added 8 commits February 11, 2020 09:08

[go-experimental] Add model constructors to initialize non-container …

cb5c54b

…vars with defaults

Add docstring and extend model_doc to include the constructor

fb7db90

Make variable names in constructor follow Go naming guidelines

a02abb5

Provide 2 different constructurs as suggested + couple fixes to gener…

6e4e538

…ate constructors right under all circumstances

Fix defaults for enums

af2e0dc

Remove erroneously added file

f854373

Properly escape vars that have reserved names

5298995

Fix after rebasing

358648b

bkabrda force-pushed the go-experimental-struct-constructor branch from 8661844 to 358648b Compare February 11, 2020 08:26

jimschubert merged commit 010dad2 into OpenAPITools:master Feb 20, 2020

Uh oh!

Conversation

bkabrda commented Jan 31, 2020

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinummonkey Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkabrda commented Feb 5, 2020

Uh oh!

bkabrda commented Feb 11, 2020

Uh oh!

bkabrda commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

platinummonkey Jan 31, 2020 •

edited

Loading