From b399e0d740e9e557988f3cfce21083fb5422ad60 Mon Sep 17 00:00:00 2001 From: Ben Kraft Date: Wed, 21 Apr 2021 19:05:38 -0700 Subject: [PATCH] move TODOs into issues --- README.md | 42 ------------------------------------------ generate/config.go | 9 +-------- generate/generate.go | 1 - generate/parse.go | 3 ++- generate/template.go | 2 -- generate/types.go | 20 +------------------- generate/util.go | 1 - graphql/client.go | 5 +---- 8 files changed, 5 insertions(+), 78 deletions(-) diff --git a/README.md b/README.md index d0c2300..35902bb 100644 --- a/README.md +++ b/README.md @@ -87,45 +87,3 @@ Khan Academy is a non-profit organization with a mission to provide a free, worl ### Design See [DESIGN.md](DESIGN.md) for documentation of major design decisions in this library. - -### Major TODOs - -(+) denotes things we further need before recommending anyone else use this in prod - -Basic features: -- redo support for interfaces, unions, fragments (see DESIGN) - -Fancy features: -- allow `__all` to request all fields (probably a terrible idea though) -- plugin to make `_entities` queries for apollo federation easy (basically you would just write the fragment) -- auto-caching -- if you `@genqlient(cache: true)` we cache (you can pass a cache into the client or something) - -Generated code customization: -- add flag(s) to make a field use a pointer (for optionality or perf; see DESIGN) -- collapsing -- should be able to have `mutation { myMutation { error { code } } }` just return `(code string, err error)` -- map a field to a particular Go type (if you want to use a named type for some string, say) -- specify a particular name for a Go type, or for the helper function, or whatever else -- include full query in generated godoc - -Config options: -- proper config/arguments setup (e.g. with [viper](https://github.com/spf13/viper)) -- (+) improve client_getter to be more usable (and document it), maybe by just saying it has to be a function that takes context, or flag it out for now. -- get schema via GraphQL introspection (although honestly, it seems like SDL is becoming popular enough there may be no need) -- whether names should be exported -- default handling for optional fields? (maybe generate a HasFoo, you can always ignore if you don't care) -- generate mocks? - -Runtime: -- (+) basic tests for graphql package -- integration tests against common servers for graphql package -- send hash rather than full query - -Internal: -- set up linting beyond `go vet` -- switch to a snapshot testing lib (cupaloy looks good) for tests - -Other: -- (+) API cleanup (check godoc) -- (+) improved validation and error checking -- (+) documentation -- get a designer to fix my bad logo-thing diff --git a/generate/config.go b/generate/config.go index f4b02b6..364891a 100644 --- a/generate/config.go +++ b/generate/config.go @@ -18,8 +18,6 @@ var defaultConfig = &Config{ type Config struct { // The filename with the GraphQL schema (in SDL format); defaults to // schema.graphql - // TODO: Allow fetching a schema via introspection (will need to figure out - // how to convert that to SDL). Schema string `yaml:"schema"` // Filenames or globs with the operations for which to generate code; @@ -64,9 +62,7 @@ type Config struct { // (which will be named ctx). For example, this might do // ctx.Value(myKey).(*graphql.Client). If omitted, client must be // passed to each method explicitly. - // TODO: what if you want to do an import in this snippet, e.g. for a - // getter function, global var, or a context-key-type? - // TODO: what if you want to return err? + // TODO(#5): This is a bit broken, fix it. ClientGetter string `yaml:"client_getter"` // A map from GraphQL scalar type name to Go fully-qualified type name for @@ -75,9 +71,6 @@ type Config struct { // to int, Float to float64, and Boolean to bool), but this setting will // extend or override those mappings. These types must define MarshalJSON // and UnmarshalJSON methods, or otherwise be convertible to JSON. - // TODO: figure out if it makes sense to say you can use these for - // non-scalar types; technically it should just work, but what if you - // didn't request the right fields? Scalars map[string]string `yaml:"scalars"` // Set automatically to the filename of the config file itself. diff --git a/generate/generate.go b/generate/generate.go index f4e5790..1e1a952 100644 --- a/generate/generate.go +++ b/generate/generate.go @@ -138,7 +138,6 @@ func (g *generator) addOperation(op *ast.OperationDefinition) error { var builder strings.Builder f := formatter.NewFormatter(&builder) - // TODO: this could even get minifed. f.FormatQueryDocument(&ast.QueryDocument{ Operations: ast.OperationList{op}, // TODO: handle fragments diff --git a/generate/parse.go b/generate/parse.go index a5f259a..a9600b1 100644 --- a/generate/parse.go +++ b/generate/parse.go @@ -53,7 +53,8 @@ func getQueries(basedir string, filenames []string) (*ast.QueryDocument, error) // in one might reference fragments in another. // // TODO(benkraft): It might be better to merge just within a filename, so - // that fragment-names don't need to be unique across files. + // that fragment-names don't need to be unique across files. (Although + // then we may have other problems; and query-names still need to be.) mergedQueryDoc := new(ast.QueryDocument) addQueryDoc := func(queryDoc *ast.QueryDocument) { mergedQueryDoc.Operations = append(mergedQueryDoc.Operations, queryDoc.Operations...) diff --git a/generate/template.go b/generate/template.go index 8f61e33..bca9415 100644 --- a/generate/template.go +++ b/generate/template.go @@ -7,8 +7,6 @@ import ( "text/template" ) -// TODO: package templates into the binary using one of those asset thingies -// (e.g. embed, if we wait until 1.16 to do this) var ( _, thisFilename, _, _ = runtime.Caller(0) thisDir = filepath.Dir(thisFilename) diff --git a/generate/types.go b/generate/types.go index e338706..e1248e5 100644 --- a/generate/types.go +++ b/generate/types.go @@ -36,7 +36,6 @@ func (g *generator) baseTypeForOperation(operation ast.Operation) (*ast.Definiti } func (g *generator) getTypeForOperation(operation *ast.OperationDefinition, queryOptions *GenqlientDirective) (name string, err error) { - // TODO: configure ResponseName format name = operation.Name + "Response" if def, ok := g.typeMap[name]; ok { @@ -90,12 +89,6 @@ func (g *generator) typeName(prefix string, typ *ast.Definition) (name, nextPref // that's the actual name (the rest are really qualifiers); but if // they are the same then including it once suffices for both // purposes.) - // TODO: do this a bit more fuzzily; for example if you have a field - // doThing: DoThingMutation - // or - // error: MyError - // we should be able to be a bit smarter than - // DoThingDoThingMutation/ErrorMyError. name += typeGoName } @@ -115,15 +108,6 @@ func (g *generator) getTypeForInputType(opName string, typ *ast.Type, options, q name := matchFirst(typ.Name(), opName) builder := &typeBuilder{generator: g} // note prefix is ignored here (see generator.typeName) - // TODO: passing options is actually kinda wrong, because it means we could - // break the "there is only Go type for each input type" rule. In practice - // it's probably rare that you use the same input type twice in a query and - // want different settings, though, and it just means we choose one or the - // other set of options. - // TODO: it's also awkward because you have no way to pass an option for an - // individual input-type field. - // TODO: should we use pointers by default for input-types if they're - // structs? err := builder.writeType(name, "", typ, selectionsForInputType(g, typ, queryOptions), options) return builder.String(), err } @@ -289,9 +273,7 @@ func (builder *typeBuilder) writeType(name, namePrefix string, typ *ast.Type, fi typ = typ.Elem } if options.GetPointer() { - // TODO: this does []*T, you might in principle want *[]T or - // *[]*T. We could add a "sliceptr" option if it comes up (that's - // still not correct if you wanted *[][]*[]T, but, like, tough luck). + // Note this does []*T or [][]*T, not e.g. *[][]T. See #16. builder.WriteString("*") } diff --git a/generate/util.go b/generate/util.go index a3d3a1c..9e4ff54 100644 --- a/generate/util.go +++ b/generate/util.go @@ -25,7 +25,6 @@ func lowerFirst(s string) string { } func upperFirst(s string) string { - // TODO: initialisms return changeFirst(strings.TrimLeft(s, "_"), unicode.ToUpper) } diff --git a/graphql/client.go b/graphql/client.go index 0f67fd4..80973e2 100644 --- a/graphql/client.go +++ b/graphql/client.go @@ -13,10 +13,7 @@ import ( // Client is the interface that the generate code calls into to actually make // requests. // -// Unstable: This interface is likely to change before v1.0. -// -// TODO: figure out what we, and others, actually want to use this for, and -// consider supporting explicit hooks for that instead. +// Unstable: This interface is likely to change before v1.0, see #19. type Client interface { // MakeRequest must make a request to the client's GraphQL API. //