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

refactor(golang): Use http provided constants for http methods #3028

Merged
merged 6 commits into from
Jun 3, 2019

Conversation

mcristina422
Copy link
Contributor

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
    @antihax @bvwells @grokify @kemokemo

Description of the PR

This modifies the golang client to use the http constants for method types as defined by RFC 7231 section 4.3. These are documented on:

https://golang.org/pkg/net/http/#pkg-constants

This does not require any new dependencies, as net/http is already imported.

Based on: #2983

@mcristina422 mcristina422 changed the title Http method refactor(golang): Use http provided constants for http methods May 29, 2019
@wing328
Copy link
Member

wing328 commented May 30, 2019

CI reports the following errors:

SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
[ERROR] Error fetching link: /home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator/home/circleci/OpenAPITools/openapi-generator/modules/openapi-generator-cli/target/apidocs. Ignored it.
# _/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go/go-petstore
go-petstore/api_another_fake.go:17:2: imported and not used: "strings"
go-petstore/api_fake.go:17:2: imported and not used: "strings"
go-petstore/api_fake_classname_tags123.go:17:2: imported and not used: "strings"
FAIL	_/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/go [build failed]
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-test) on project Goswagger: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.

Looks like the strings import needs to be removed?

@mcristina422
Copy link
Contributor Author

Oh, the edge case comes into play when the api file gets rendered with a path that includes a path parameter. Since sometimes this exists (or doesn't exist) for multiple operations, the logic of including this is not trivial.

Also fmt should probably also be moved into that block in
3deaa80 since fmt and strings both are only used in that case.

This is my first real interaction with mustache and this edge case seems more complex to handle than I initially thought.

Maybe the simplest solution is to always import it and hush the compiler with

// Linger please
var (
	_ context.Context
        _ strings.Replacer
)

Thoughts?

Also what does this do exactly? Maybe I'm misunderstanding something where this could be used

{{#imports}}	"{{import}}"
{{/imports}}

@wing328
Copy link
Member

wing328 commented May 31, 2019

The imports is for importing models (e.g. Pet) defined in the endpoint/operation.

@wing328
Copy link
Member

wing328 commented May 31, 2019

This is my first real interaction with mustache and this edge case seems more complex to handle than I initially thought.

Yup, that's another edge case that I've not yet encountered with other generators.

As a workaround, what about using "strings" somehow in the code so as to hardcode the import?

@mcristina422
Copy link
Contributor Author

Ah, the import logic is handled here

// this will only import "fmt" and "strings" if there are items in pathParams
for (CodegenOperation operation : operations) {
if (operation.pathParams != null && operation.pathParams.size() > 0) {
imports.add(createMapping("import", "fmt"));
imports.add(createMapping("import", "strings"));
break; //just need to import once
}
}
so just adding strings to that was enough. No workarounds needed

@wing328 wing328 added this to the 4.0.2 milestone Jun 3, 2019
@wing328 wing328 merged commit 8d9eb5b into OpenAPITools:master Jun 3, 2019
@wing328
Copy link
Member

wing328 commented Jun 20, 2019

Thanks again for the PR, which has been included in the v4.0.2 release: https://twitter.com/oas_generator/status/1141610197766426626

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

2 participants