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

[Golang][client] Add option for standard Go generated code comment #555

Merged

Conversation

grokify
Copy link
Member

@grokify grokify commented Jul 13, 2018

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.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@antihax
@bvwells

Description of the PR

Update the generated code comment to the following official Go pattern:

^// Code generated .* DO NOT EDIT\.$

This allows the auto-generated code to be differentially processed by Go Lint as discussed here:

This is also used by Go Report Card.

@grokify grokify changed the title [Golang][client] Update to standard generated comment [Golang][client] Update to standard generated code comment Jul 13, 2018
@etherealjoy
Copy link
Contributor

will this prevent warning of un-commented Exported symbols?

@grokify
Copy link
Member Author

grokify commented Jul 13, 2018

will this prevent warning of un-commented Exported symbols?

Standard Go behavior is that uncommented, exported symbol warnings will be suppressed when the machine-generated code comment is present. The files will be ignored by golint, and thus gometalinter. Whether or not this behavior is desirable is situational. For many scenarios, I think linter comment warnings are a good thing but a number of people have presented cases where it is not desirable in golang/lint#263.

Perhaps we could add a flag to allow builders to decide whether or not they want golint support / Go style comment.

@grokify
Copy link
Member Author

grokify commented Jul 14, 2018

Added generator flag withGoCodegenComment to enable Go code generation comment when set to true.

I left the default with the original comment which will allow golint to work because although the standard Go behavior is to disable linting with auto-generated code, I think it's worthwhile for code humans need to use.

Tested with:

  1. Build: java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -t modules/openapi-generator/src/main/resources/go -i modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -g go -o samples/client/petstore/go/go-petstore -DpackageName=petstore,withGoCodegenComment=true
  2. Help: java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar config-help -g go

@etherealjoy
Copy link
Contributor

etherealjoy commented Jul 14, 2018

I think it's worthwhile for code humans need to use.

This is what I meant. It is not always ideal to suppress warnings.

@grokify
Copy link
Member Author

grokify commented Jul 14, 2018

I think it's worthwhile for code humans need to use.

This is what I meant. It is not always ideal to suppress warnings.

Agree. It seems like there are lots of opinions on this on both sides given different use cases for machine-generated code. The updated PR supports this feature as a optional flag so people of both persuasions can be supported.

I updated the samples so go-petstore-withXml sample has -DwithGoCodegenComment=true. go-petstore continues to have the default behavior. That way both behaviors can be seen in the samples.

Regarding the current generator errors, it seems like there are at least two golint opportunities here.

  1. Comments

The following type of error looks like simple improvement opportunities for our templates, e.g. response.mustache.

client/response.go:16:6: exported type APIResponse should have comment or be unexported
client/response.go:33:1: exported function NewAPIResponse should have comment or be unexported
client/response.go:39:1: exported function NewAPIResponseWithError should have comment or be unexported
  1. Capitalization Convention

The following is an algorithm update. This will also make the client SDK more consistent because APIClient is not ApiClient so converting user_id to userID would make the generated code more consistent as well.

examples/scim_users_get/get_scim_users.go:217:3: var userId should be userID

I've created issue #565 to track resolution of these, in addition to gofmt -s.

@grokify grokify changed the title [Golang][client] Update to standard generated code comment [Golang][client] Add option for standard Go generated code comment Jul 15, 2018
@@ -13,5 +13,11 @@
{{#infoEmail}}
* Contact: {{{infoEmail}}}
{{/infoEmail}}
{{^withGoCodegenComment}}
* Generated by: OpenAPI Generator (https://openapi-generator.tech)
Copy link
Member

Choose a reason for hiding this comment

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

@grokify thanks for the PR. What about replacing this line directly with the following to avoid adding another option?

// Code generated by OpenAPI Generator (https://openapi-generator.tech); DO NOT EDIT.

Copy link
Member Author

@grokify grokify Jul 18, 2018

Choose a reason for hiding this comment

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

My original commit f31ecad was a simple replace, however, in discussion with @etherealjoy, I agreed it's not ideal to remove the original behavior and that the original behavior should be the default. This is because this Go behavior change seems more appropriate for machine-generated code that humans aren't meant to use or code where there are no resources for documentation. This trigger's Go's special linter handling for machine generated code and disables golint and thus gometalinter. The result is that:

  1. Warnings for exported symbols without comments will be suppressed meaning lack of comments will not be identified
  2. Warnings for non-standard MixedCase symbol names will be suppressed

For exported symbol comments, it seems generally better to have comments since generated SDKs are expected to be used by humans. Depending on the specification, this can be the majority of warnings. Comments are important in Go because they are automatically displayed in documentation tools like GoDoc similar to the official Go docs. The cases where people indicated a preference for suppression of these warnings seem to be related to when there are no documentation resources available for a legacy / deprecated API. While it’s nice to support legacy / deprecated API use cases, it doesn’t feel like it should be the default behavior.

MixedCase is a different issue since sometimes I personally feel MixedCase linter is too strict. I like names like userId and serverUrl because it makes the symbol name more easily parseable and split into "concepts", but I recognize the standard in the Go community here.

There is a way to separate the comment and case behavior, but that involves having golint issue the warnings and then using gometalinter to selectively filter the warnings you are not interested in.

So, the thought is to make the comment a non-default option because it will:

  1. allow users to see comment errors which should be addressed most of the time
  2. support more customized behavior using a combination of golint and gometalinter
  3. support different behavior for scenarios like legacy / deprecated API use cases

Copy link
Member

Choose a reason for hiding this comment

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

@grokify thanks for the explanation. My feedback was to avoid adding too many options for a generator. I'm ok with this new option given the reason you provided.

@wing328 wing328 merged commit 0f0d8a0 into OpenAPITools:master Jul 19, 2018
@wing328 wing328 added this to the 3.1.2 milestone Jul 19, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…penAPITools#555)

* update go generated code comment

* update samples for go petstore

* update code generation comment

* update samples for go petstore

* Trigger CI due to previous Shippable race condition

* add -DwithGoCodegenComment=true option

* reset samples

* add withGoCodegenComment=true to bin/go-petstore-withxml.sh

* update samples/.../go-petstore-withXml using -DwithGoCodegenComment=true
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.

None yet

3 participants