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

[Go] Conform to Golang idioms and best practices #3518

Closed
neilotoole opened this issue Aug 4, 2016 · 3 comments
Closed

[Go] Conform to Golang idioms and best practices #3518

neilotoole opened this issue Aug 4, 2016 · 3 comments

Comments

@neilotoole
Copy link
Contributor

Description

There are several locations where the generated Go code does not conform to standard Go idioms / conventions / best practices. Taking CodeReviewComments as a primary source, and generally trying to follow the standards in the Go std lib and in popular OSS libraries.

For example:

  • Use proper case: e.g. a generated object named ProjectApi should be ProjectAPI.
  • The Configuration object should be named Config.
  • Do we need to export APIClient at all? What's the use case for having this public?
Swagger-codegen version

master

@wing328
Copy link
Contributor

wing328 commented Aug 9, 2016

@neilotoole sounds good to me as long as it conforms to the "Go Code Review Comments"

cc @guohuang

@vbatts
Copy link
Contributor

vbatts commented Oct 13, 2017

discovered a small bit on swagger-codegen-cli-2.2.3.jar and fixed in #6669
There is still an issue of "how to populate the infoEmail field?" so there is not a trialing whitespace on that line as well.

But after building with master, it looks like the refactor for 2.3.0 introduced new issues.
A useful tool in checking is gofmt or go get golang.org/x/tools/cmd/goimports

  1. indentation of imports, and placement of non-stdlib paths should be at the end of the import set:
    This is the diff of 2.2.3 -> 2.3.0-SNAPSHOT (96444d3)
@@ -12,22 +12,69 @@ package api
 
 import (
        "bytes"
+       "encoding/json"
+       "encoding/xml"
        "fmt"
-       "io/ioutil"
+       "errors"
+       "io"
+       "mime/multipart"
+    "golang.org/x/oauth2"
+    "golang.org/x/net/context"
+       "net/http"
        "net/url"
+       "time"
+       "os"
        "path/filepath"
        "reflect"
+       "regexp"
        "strings"
+       "unicode/utf8"
+       "strconv"
+)

it ought to be like

import (
        "bytes"
        "encoding/json"
        "encoding/xml"
        "errors"
        "fmt"
        "io"
        "mime/multipart"
        "net/http"
        "net/url"
        "os"
        "path/filepath"
        "reflect"
        "regexp"
        "strconv"
        "strings"
        "time"
        "unicode/utf8"

        "golang.org/x/net/context"
        "golang.org/x/oauth2"
)
  1. alignment: these variable assignment align to the "=" sign.
+var (
+       jsonCheck = regexp.MustCompile("(?i:[application|text]/json)")
+       xmlCheck = regexp.MustCompile("(?i:[application|text]/xml)")
 )

it ought to be like

        jsonCheck = regexp.MustCompile("(?i:[application|text]/json)")
        xmlCheck  = regexp.MustCompile("(?i:[application|text]/xml)")

This is true for fields of a struct as well, they align to the type, as well as the tag on the field.
Here is a diff of the corrected output from 2.3.0-SNAPSHOT after goimports -w ...

 type Configuration struct {
-       BasePath      string                    `json:"basePath,omitempty"`
-       Host          string                    `json:"host,omitempty"`
-       Scheme        string                    `json:"scheme,omitempty"`
-       DefaultHeader map[string]string         `json:"defaultHeader,omitempty"`
-       UserAgent     string                    `json:"userAgent,omitempty"`
-       HTTPClient        *http.Client
+       BasePath      string            `json:"basePath,omitempty"`
+       Host          string            `json:"host,omitempty"`
+       Scheme        string            `json:"scheme,omitempty"`
+       DefaultHeader map[string]string `json:"defaultHeader,omitempty"`
+       UserAgent     string            `json:"userAgent,omitempty"`
+       HTTPClient    *http.Client
 }
  1. no space between function name and parenthesis in function signature.
    Here is a diff from 2.3.0-SNAPSHOT after goimports -w ...:
 // Change base path to allow switching to mocks
-func (c *APIClient) ChangeBasePath (path string) {
+func (c *APIClient) ChangeBasePath(path string) {
        c.cfg.BasePath = path
  1. single variable returns in a function do not need parenthesis:
    Here is a diff from 2.3.0-SNAPSHOT after goimports -w ...:
 // Prevent trying to import "fmt"
-func reportError(format string, a ...interface{}) (error) {
+func reportError(format string, a ...interface{}) error {
        return fmt.Errorf(format, a...)
  1. initialized objects with empty curly brackets need no space.
    Here is a diff from 2.3.0-SNAPSHOT after goimports -w ...:
        // to determine the Content-Type header
-       localVarHttpContentTypes := []string{  }
+       localVarHttpContentTypes := []string{}
 
        // set Content-Type header
        localVarHttpContentType := selectHeaderContentType(localVarHttpContentTypes)
@@ -69,8 +69,7 @@ func (a *GrafeasApiService) CreateNote(projectsId string, localVarOptionals map[
        }
 
        // to determine the Accept header
-       localVarHttpHeaderAccepts := []string{
-               }
+       localVarHttpHeaderAccepts := []string{}

@wing328
Copy link
Contributor

wing328 commented Oct 16, 2017

@vbatts I've filed #6695 to fix a few formatting issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants