Skip to content

Commit

Permalink
Bump golangci-lint and (max) Go versions (#219)
Browse files Browse the repository at this point in the history
Lint is failing with some inscrutable panic (on a commit with no code
changes). Let's try bumping the version in case they fixed it.

Additionally, the new version's github action uses Go 1.19, which means
it pulls in gofmt updates to match the new [doc-comment formatting rules][1].
So I added Go 1.19 to our list of versions to test (fixes #216)
and updated some of our doc-comments to format better in the
new world (mostly using the new link syntax).

[1]: https://go.dev/doc/comment

Test plan: make lint
  • Loading branch information
benjaminjkraft authored Aug 15, 2022
1 parent c0510ff commit f600b6e
Show file tree
Hide file tree
Showing 16 changed files with 396 additions and 196 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
go: [ '1.16', '1.17', '1.18' ]
go: [ '1.16', '1.17', '1.18', '1.19' ]

steps:
- name: Set up Go
Expand Down Expand Up @@ -42,4 +42,4 @@ jobs:
- name: Run lint
uses: golangci/golangci-lint-action@v2
with:
version: v1.45 # should match internal/lint/go.mod
version: v1.48 # should match internal/lint/go.mod
18 changes: 11 additions & 7 deletions generate/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ var cfgFilenames = []string{".genqlient.yml", ".genqlient.yaml", "genqlient.yml"
// Config represents genqlient's configuration, generally read from
// genqlient.yaml.
//
// Callers must call ValidateAndFillDefaults before using the config.
// Callers must call [Config.ValidateAndFillDefaults] before using the config.
type Config struct {
// The following fields are documented at:
// https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
// The following fields are documented in the [genqlient.yaml docs].
//
// [genqlient.yaml docs]: https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
Schema StringList `yaml:"schema"`
Operations StringList `yaml:"operations"`
Generated string `yaml:"generated"`
Expand All @@ -46,8 +47,9 @@ type Config struct {
}

// A TypeBinding represents a Go type to which genqlient will bind a particular
// GraphQL type, and is documented further at:
// https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
// GraphQL type, and is documented further in the [genqlient.yaml docs].
//
// [genqlient.yaml docs]: https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
type TypeBinding struct {
Type string `yaml:"type"`
ExpectExactFields string `yaml:"expect_exact_fields"`
Expand All @@ -56,8 +58,10 @@ type TypeBinding struct {
}

// A PackageBinding represents a Go package for which genqlient will
// automatically generate TypeBindings, and is documented further at:
// https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
// automatically generate [TypeBinding] values, and is documented further in
// the [genqlient.yaml docs].
//
// [genqlient.yaml docs]: https://github.com/Khan/genqlient/blob/main/docs/genqlient.yaml
type PackageBinding struct {
Package string `yaml:"package"`
}
Expand Down
23 changes: 13 additions & 10 deletions generate/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,12 +424,12 @@ func (g *generator) convertDefinition(
goName := upperFirst(field.Name)
// Several of the arguments don't really make sense here:
// (note field.Type is necessarily a scalar, input, or enum)
// - namePrefix is ignored for input types and enums (see
// names.go) and for scalars (they use client-specified
// names)
// - selectionSet is ignored for input types, because we
// just use all fields of the type; and it's nonexistent
// for scalars and enums, our only other possible types
// - namePrefix is ignored for input types and enums (see
// names.go) and for scalars (they use client-specified
// names)
// - selectionSet is ignored for input types, because we
// just use all fields of the type; and it's nonexistent
// for scalars and enums, our only other possible types
// TODO(benkraft): Can we refactor to avoid passing the values that
// will be ignored? We know field.Type is a scalar, enum, or input
// type. But plumbing that is a bit tricky in practice.
Expand Down Expand Up @@ -650,23 +650,26 @@ func (g *generator) convertSelectionSet(
// of the given type", which is true when the given type is or implements
// the fragment's type. This is distinct from the rules for when a fragment
// spread is legal, which is true when the fragment would be active for *any*
// of the concrete types the spread-context could have (see
// https://spec.graphql.org/draft/#sec-Fragment-Spreads or docs/DESIGN.md).
// of the concrete types the spread-context could have (see the [GraphQL spec]
// or docs/DESIGN.md).
//
// containingTypedef is as described in convertInlineFragment, below.
// fragmentTypedef is the definition of the fragment's type-condition, i.e. the
// definition of MyType in a fragment `on MyType`.
//
// [GraphQL spec]: https://spec.graphql.org/draft/#sec-Fragment-Spreads
func fragmentMatches(containingTypedef, fragmentTypedef *ast.Definition) bool {
if containingTypedef.Name == fragmentTypedef.Name {
return true
}
for _, iface := range containingTypedef.Interfaces {
// Note we don't need to recurse into the interfaces here, because in
// GraphQL types must list all the interfaces they implement, including
// all types those interfaces implement [1]. Actually, at present
// all types those interfaces implement ([spec]). Actually, at present
// gqlparser doesn't even support interfaces implementing other
// interfaces, but our code would handle that too.
// [1] https://spec.graphql.org/draft/#sec-Interfaces.Interfaces-Implementing-Interfaces
//
// [spec]: https://spec.graphql.org/draft/#sec-Interfaces.Interfaces-Implementing-Interfaces
if iface == fragmentTypedef.Name {
return true
}
Expand Down
6 changes: 3 additions & 3 deletions generate/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,9 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error {
// Generate is the main programmatic entrypoint to genqlient, and generates and
// returns Go source code based on the given configuration.
//
// See Config for more on creating a configuration. The return value is a map
// from filename to the generated file-content (e.g. Go source). Callers who
// don't want to manage reading and writing the files should call Main.
// See [Config] for more on creating a configuration. The return value is a
// map from filename to the generated file-content (e.g. Go source). Callers
// who don't want to manage reading and writing the files should call [Main].
func Generate(config *Config) (map[string][]byte, error) {
// Step 1: Read in the schema and operations from the files defined by the
// config (and validate the operations against the schema). This is all
Expand Down
2 changes: 2 additions & 0 deletions generate/genqlient_directive.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,10 @@ func setString(optionName string, dst *string, v *ast.Value, pos *ast.Position)
//
// If there are multiple genqlient directives are applied to the same node,
// e.g.
//
// # @genqlient(...)
// # @genqlient(...)
//
// add will be called several times. In this case, conflicts between the
// options are an error.
func (dir *genqlientDirective) add(graphQLDirective *ast.Directive, pos *ast.Position) error {
Expand Down
20 changes: 12 additions & 8 deletions generate/imports.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,24 @@ var _sliceOrMapPrefixRegexp = regexp.MustCompile(`^(\*|\[\d*\]|map\[string\])*`)
//
// Ideally, we want to allow a reference to basically an arbitrary symbol.
// But that's very hard, because it might be quite complicated, like
//
// struct{ F []map[mypkg.K]otherpkg.V }
//
// Now in practice, using an unnamed struct is not a great idea, but we do
// want to allow as much as we can that encoding/json knows how to work
// with, since you would reasonably expect us to accept, say,
// map[string][]interface{}. So we allow:
// - any named type (mypkg.T)
// - any predeclared basic type (string, int, etc.)
// - interface{}
// - for any allowed type T, *T, []T, [N]T, and map[string]T
// - any named type (mypkg.T)
// - any predeclared basic type (string, int, etc.)
// - interface{}
// - for any allowed type T, *T, []T, [N]T, and map[string]T
//
// which effectively excludes:
// - unnamed struct types
// - map[K]V where K is a named type wrapping string
// - any nonstandard spelling of those (interface {/* hi */},
// map[ string ]T)
// - unnamed struct types
// - map[K]V where K is a named type wrapping string
// - any nonstandard spelling of those (interface {/* hi */},
// map[ string ]T)
//
// (This is documented in docs/genqlient.yaml)
func (g *generator) ref(fullyQualifiedName string) (qualifiedName string, err error) {
errorMsg := `invalid type-name "%v" (%v); expected a builtin, ` +
Expand Down
10 changes: 7 additions & 3 deletions generate/main.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Package generate provides programmatic access to genqlient's functionality,
// and documentation of its configuration options. For general usage
// documentation, see github.com/Khan/genqlient.
// documentation, see the project [GitHub].
//
// [GitHub]: https://github.com/Khan/genqlient
package generate

import (
Expand Down Expand Up @@ -62,8 +64,10 @@ See https://github.com/Khan/genqlient for full documentation.
}

// Main is the command-line entrypoint to genqlient; it's equivalent to calling
// `go run github.com/Khan/genqlient`. For lower-level control over
// genqlient's operation, see Generate.
//
// go run github.com/Khan/genqlient
//
// For lower-level control over genqlient's operation, see [Generate].
func Main() {
exitIfError := func(err error) {
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion generate/names.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,11 @@ package generate
// can collide.
// All cases seem fairly rare in practice; eventually we'll likely allow users
// the ability to specify their own names, which they could use to avoid this
// (see https://github.com/Khan/genqlient/issues/12).
// (see [issue #12]).
// TODO(benkraft): We should probably at least try to detect it and bail.
//
// [issue #12]: https://github.com/Khan/genqlient/issues/12
//
// To implement all of the above, as we traverse the operation (and schema) in
// convert.go, we keep track of a list of parts to prefix to our type-names.
// The list always ends with a field, not a type; and we extend it when
Expand Down
5 changes: 3 additions & 2 deletions generate/stringlist.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package generate

// StringList provides yaml unmarshaler to accept both `string` and `[]string` as a valid type.
// Sourced from:
// https://github.com/99designs/gqlgen/blob/1a0b19feff6f02d2af6631c9d847bc243f8ede39/codegen/config/config.go#L302-L329
// Sourced from [gqlgen].
//
// [gqlgen]: https://github.com/99designs/gqlgen/blob/1a0b19feff6f02d2af6631c9d847bc243f8ede39/codegen/config/config.go#L302-L329
type StringList []string

func (a *StringList) UnmarshalYAML(unmarshal func(interface{}) error) error {
Expand Down
27 changes: 17 additions & 10 deletions generate/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,11 @@ func (field *goStructField) Selector() string {
}

// unmarshaler returns:
// - the name of the function to use to unmarshal this field
// - true if this is a fully-qualified name (false if it is a package-local
// unqualified name)
// - true if we need to generate an unmarshaler at all, false if the default
// behavior will suffice
// - the name of the function to use to unmarshal this field
// - true if this is a fully-qualified name (false if it is a package-local
// unqualified name)
// - true if we need to generate an unmarshaler at all, false if the default
// behavior will suffice
func (field *goStructField) unmarshaler() (qualifiedName string, needsImport bool, needsUnmarshaler bool) {
switch typ := field.GoType.Unwrap().(type) {
case *goOpaqueType:
Expand All @@ -214,9 +214,9 @@ func (field *goStructField) Unmarshaler(g *generator) (string, error) {
}

// marshaler returns:
// - the fully-qualified name of the function to use to marshal this field
// - true if we need to generate an marshaler at all, false if the default
// behavior will suffice
// - the fully-qualified name of the function to use to marshal this field
// - true if we need to generate an marshaler at all, false if the default
// behavior will suffice
func (field *goStructField) marshaler() (qualifiedName string, needsImport bool, needsMarshaler bool) {
switch typ := field.GoType.Unwrap().(type) {
case *goOpaqueType:
Expand Down Expand Up @@ -273,17 +273,21 @@ type selector struct {
// and the paths to reach them (via those embeds), but with different
// visibility rules for conflicting fields than Go.
//
// (Before you read further, now's a good time to review Go's rules:
// https://golang.org/ref/spec#Selectors. Done? Good.)
// (Before you read further, now's a good time to review [Go's rules].
// Done? Good.)
//
// To illustrate the need, consider the following query:
//
// fragment A on T { id }
// fragment B on T { id }
// query Q { t { ...A ...B } }
//
// We generate types:
//
// type A struct { Id string `json:"id"` }
// type B struct { Id string `json:"id"` }
// type QT struct { A; B }
//
// According to Go's embedding rules, QT has no field Id: since QT.A.Id and
// QT.B.Id are at equal depth, neither wins and gets promoted. (Go's JSON
// library uses similar logic to decide which field to write to JSON, except
Expand All @@ -307,7 +311,10 @@ type selector struct {
// practice, hopefully, they all match, but validating that is even more work
// for a fairly rare case.) This function returns, for each JSON-name, the Go
// field we want to use. In the example above, it would return:
//
// []selector{{<goStructField for QT.A.Id>, "A.Id"}}
//
// [Go's rules]: https://golang.org/ref/spec#Selectors
func (typ *goStructType) FlattenedFields() ([]*selector, error) {
seenJSONNames := map[string]bool{}
retval := make([]*selector, 0, len(typ.Fields))
Expand Down
42 changes: 27 additions & 15 deletions graphql/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,31 +46,39 @@ type client struct {
method string
}

// NewClient returns a Client which makes requests to the given endpoint,
// NewClient returns a [Client] which makes requests to the given endpoint,
// suitable for most users.
//
// The client makes POST requests to the given GraphQL endpoint using standard
// GraphQL HTTP-over-JSON transport. It will use the given http client, or
// http.DefaultClient if a nil client is passed.
// GraphQL HTTP-over-JSON transport. It will use the given [http.Client], or
// [http.DefaultClient] if a nil client is passed.
//
// The typical method of adding authentication headers is to wrap the client's
// Transport to add those headers. See example/caller.go for an example.
// [http.Transport] to add those headers. See [example/main.go] for an
// example.
//
// [example/main.go]: https://github.com/Khan/genqlient/blob/main/example/main.go#L12-L20
func NewClient(endpoint string, httpClient Doer) Client {
return newClient(endpoint, httpClient, http.MethodPost)
}

// NewClientUsingGet returns a Client which makes requests to the given endpoint,
// suitable for most users.
// NewClientUsingGet returns a [Client] which makes GET requests to the given
// endpoint suitable for most users who wish to make GET requests instead of
// POST.
//
// The client makes GET requests to the given GraphQL endpoint using a GET query,
// with the query, operation name and variables encoded as URL parameters.
// It will use the given http client, or http.DefaultClient if a nil client is passed.
// The client makes GET requests to the given GraphQL endpoint using a GET
// query, with the query, operation name and variables encoded as URL
// parameters. It will use the given [http.Client], or [http.DefaultClient] if
// a nil client is passed.
//
// The client does not support mutations, and will return an error if passed a request
// that attempts one.
// The client does not support mutations, and will return an error if passed a
// request that attempts one.
//
// The typical method of adding authentication headers is to wrap the client's
// Transport to add those headers. See example/caller.go for an example.
// [http.Transport] to add those headers. See [example/main.go] for an
// example.
//
// [example/main.go]: https://github.com/Khan/genqlient/blob/main/example/main.go#L12-L20
func NewClientUsingGet(endpoint string, httpClient Doer) Client {
return newClient(endpoint, httpClient, http.MethodGet)
}
Expand All @@ -82,18 +90,20 @@ func newClient(endpoint string, httpClient Doer, method string) Client {
return &client{httpClient, endpoint, method}
}

// Doer encapsulates the methods from *http.Client needed by Client.
// The methods should have behavior to match that of *http.Client
// Doer encapsulates the methods from [*http.Client] needed by [Client].
// The methods should have behavior to match that of [*http.Client]
// (or mocks for the same).
type Doer interface {
Do(*http.Request) (*http.Response, error)
}

// Request contains all the values required to build queries executed by
// the graphql.Client.
// the [Client].
//
// Typically, GraphQL APIs will accept a JSON payload of the form
//
// {"query": "query myQuery { ... }", "variables": {...}}`
//
// and Request marshals to this format. However, MakeRequest may
// marshal the data in some other way desired by the backend.
type Request struct {
Expand All @@ -112,7 +122,9 @@ type Request struct {
// Response that contains data returned by the GraphQL API.
//
// Typically, GraphQL APIs will return a JSON payload of the form
//
// {"data": {...}, "errors": {...}}
//
// It may additionally contain a key named "extensions", that
// might hold GraphQL protocol extensions. Extensions and Errors
// are optional, depending on the values returned by the server.
Expand Down
16 changes: 12 additions & 4 deletions graphql/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@ package graphql
//
// It is used to prevent a struct type from inheriting its embed's
// UnmarshalJSON method, so if we construct a type:
//
// type T struct { E; NoUnmarshalJSON }
// where E has an UnmarshalJSON method, T will not inherit it, per the Go
// selector rules: https://golang.org/ref/spec#Selectors.
//
// where E has an UnmarshalJSON method, T will not inherit it, per the
// [Go selector rules].
//
// [Go selector rules]: https://golang.org/ref/spec#Selectors.
type NoUnmarshalJSON struct{}

// UnmarshalJSON should never be called; it exists only to prevent a sibling
Expand All @@ -22,9 +26,13 @@ func (NoUnmarshalJSON) UnmarshalJSON(b []byte) error {
//
// It is used to prevent a struct type from inheriting its embed's
// MarshalJSON method, so if we construct a type:
//
// type T struct { E; NoMarshalJSON }
// where E has an MarshalJSON method, T will not inherit it, per the Go
// selector rules: https://golang.org/ref/spec#Selectors.
//
// where E has an MarshalJSON method, T will not inherit it, per the
// [Go selector rules].
//
// [Go selector rules]: https://golang.org/ref/spec#Selectors.
type NoMarshalJSON struct{}

// MarshalJSON should never be called; it exists only to prevent a sibling
Expand Down
Loading

0 comments on commit f600b6e

Please sign in to comment.