From 7794c1b7a6e258bd47e1e8217ce469d37d779116 Mon Sep 17 00:00:00 2001
From: Ben Kraft <benkraft@khanacademy.org>
Date: Wed, 22 Sep 2021 17:08:53 -0700
Subject: [PATCH] Review comments

---
 generate/convert.go        | 17 ++++++++++++-----
 generate/operation.go.tmpl |  7 +++++--
 graphql/client.go          |  2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/generate/convert.go b/generate/convert.go
index 84e3f297..23981664 100644
--- a/generate/convert.go
+++ b/generate/convert.go
@@ -161,8 +161,10 @@ func (g *generator) convertArguments(
 		options := queryOptions.merge(directive)
 
 		goName := upperFirst(arg.Variable)
-		// note prefix is ignored here (see generator.typeName), as is
-		// selectionSet (for input types we use the whole thing).
+		// Some of the arguments don't apply here, namely the name-prefix (see
+		// names.go) and the selection-set (we use all the input type's fields,
+		// and so on recursively).  See also the `case ast.InputObject` in
+		// convertDefinition, below.
 		goTyp, err := g.convertType(nil, arg.Type, nil, options, queryOptions)
 		if err != nil {
 			return nil, err
@@ -354,11 +356,16 @@ func (g *generator) convertDefinition(
 
 		for i, field := range def.Fields {
 			goName := upperFirst(field.Name)
-			// Several of the arguments don't really make sense here:
+			// Several of the arguments don't really make sense here
+			// (note field.Type is necessarily a scalar, input, or enum)
 			// - no field-specific options can apply, because this is
 			//   a field in the type, not in the query (see also #14).
-			// - namePrefix is ignored for input types; see note in
-			//   generator.typeName.
+			// - 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.
diff --git a/generate/operation.go.tmpl b/generate/operation.go.tmpl
index 80b0e43c..47a0c939 100644
--- a/generate/operation.go.tmpl
+++ b/generate/operation.go.tmpl
@@ -8,13 +8,16 @@ func {{.Name}}(
     {{end}}
     {{- if .Input -}}
     {{- range .Input.Fields -}}
-    {{/* the GraphQL name is here the user-specified variable-name */ -}}
+    {{/* the GraphQL name here is the user-specified variable-name */ -}}
     {{.GraphQLName}} {{.GoType.Reference}},
     {{end -}}
     {{end -}}
 ) (*{{.ResponseName}}, error) {
     {{- if .Input -}}
-    {{/* __input is not a legal variable name in GraphQL */ -}}
+    {{/* We need to avoid conflicting with any of the function's argument names
+         which are derived from the GraphQL argument names; notably `input` is
+         a common one.  So we use a name that's not legal in GraphQL, namely
+         one starting with a double-underscore. */ -}}
     __input := {{.Input.GoName}}{
         {{range .Input.Fields -}}
         {{.GoName}}: {{.GraphQLName}},
diff --git a/graphql/client.go b/graphql/client.go
index fdd106c0..db7b8269 100644
--- a/graphql/client.go
+++ b/graphql/client.go
@@ -26,7 +26,7 @@ type Client interface {
 	// query is the literal string representing the GraphQL query, e.g.
 	// `query myQuery { myField }`.  variables contains a JSON-marshalable
 	// value containing the variables to be sent along with the query,
-	// or may be nil if there are none.  Typically, GraphQL APIs wills
+	// or may be nil if there are none.  Typically, GraphQL APIs will
 	// accept a JSON payload of the form
 	//	{"query": "query myQuery { ... }", "variables": {...}}`
 	// but MakeRequest may use some other transport, handle extensions, or set