Implement missing edge cases in queryArguments. (#7)

Previously, queryArguments was partially implemented to handle the most
common types I had run into, with a TODO to complete the rest.

This change implements the support for the remaining argument types.
Now, slices of T are mapped to "[T]!", and pointers to slices of T are
mapped to "[T]", as per GraphQL specification.

Add a comment pointing out that commas are omitted because they're
insignificant, and we're looking to produce minified output (less bytes
to send over the network).

Fixes #6.

query.go

@@ -30,33 +30,56 @@ func constructMutation(v interface{}, variables map[string]interface{}) string {
 //
 // E.g., map[string]interface{}{"a": Int(123), "b": NewBoolean(true)} -> "$a:Int!$b:Boolean".
 func queryArguments(variables map[string]interface{}) string {
-	sorted := make([]string, 0, len(variables))
+	// Sort keys in order to produce deterministic output for testing purposes.
+	// TODO: If tests can be made to work with non-deterministic output, then no need to sort.
+	keys := make([]string, 0, len(variables))
 	for k := range variables {
-		sorted = append(sorted, k)
+		keys = append(keys, k)
 	}
-	sort.Strings(sorted)
-	var s string
-	for _, k := range sorted {
-		v := variables[k]
-		s += "$" + k + ":"
-		t := reflect.TypeOf(v)
-		switch t.Kind() {
-		case reflect.Slice, reflect.Array:
-			// TODO: Support t.Elem() being a pointer, if needed. Probably want to do this recursively.
-			s += "[" + t.Elem().Name() + "!]" // E.g., "[IssueState!]".
-		case reflect.Ptr:
-			// Pointer is an optional type, so no "!" at the end.
-			s += t.Elem().Name() // E.g., "Int".
-		default:
-			name := t.Name()
-			if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubql/issues/12.
-				name = "ID"
-			}
-			// Value is a required type, so add "!" to the end.
-			s += name + "!" // E.g., "Int!".
+	sort.Strings(keys)
+
+	var buf bytes.Buffer
+	for _, k := range keys {
+		io.WriteString(&buf, "$")
+		io.WriteString(&buf, k)
+		io.WriteString(&buf, ":")
+		writeArgumentType(&buf, reflect.TypeOf(variables[k]), true)
+		// Don't insert a comma here.
+		// Commas in GraphQL are insignificant, and we want minified output.
+		// See https://facebook.github.io/graphql/October2016/#sec-Insignificant-Commas.
+	}
+	return buf.String()
+}
+
+// writeArgumentType writes a minified GraphQL type for t to w.
+// value indicates whether t is a value (required) type or pointer (optional) type.
+// If value is true, then "!" is written at the end of t.
+func writeArgumentType(w io.Writer, t reflect.Type, value bool) {
+	if t.Kind() == reflect.Ptr {
+		// Pointer is an optional type, so no "!" at the end of the pointer's underlying type.
+		writeArgumentType(w, t.Elem(), false)
+		return
+	}
+
+	switch t.Kind() {
+	case reflect.Slice, reflect.Array:
+		// List. E.g., "[Int]".
+		io.WriteString(w, "[")
+		writeArgumentType(w, t.Elem(), true)
+		io.WriteString(w, "]")
+	default:
+		// Named type. E.g., "Int".
+		name := t.Name()
+		if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubql/issues/12.
+			name = "ID"
 		}
+		io.WriteString(w, name)
+	}
+
+	if value {
+		// Value is a required type, so add "!" to the end.
+		io.WriteString(w, "!")
 	}
-	return s
 }
 
 // query uses writeQuery to recursively construct
@@ -69,8 +92,8 @@ func query(v interface{}) string {
 	return buf.String()
 }
 
-// writeQuery writes a minified query for t to w. If inline is true,
-// the struct fields of t are inlined into parent struct.
+// writeQuery writes a minified query for t to w.
+// If inline is true, the struct fields of t are inlined into parent struct.
 func writeQuery(w io.Writer, t reflect.Type, inline bool) {
 	switch t.Kind() {
 	case reflect.Ptr, reflect.Slice:

query_test.go

@@ -273,7 +273,6 @@ func TestConstructMutation(t *testing.T) {
 
 func TestQueryArguments(t *testing.T) {
 	tests := []struct {
-		name string
 		in   map[string]interface{}
 		want string
 	}{
@@ -282,26 +281,43 @@ func TestQueryArguments(t *testing.T) {
 			want: "$a:Int!$b:Boolean",
 		},
 		{
-			in:   map[string]interface{}{"states": []IssueState{IssueStateOpen, IssueStateClosed}},
-			want: "$states:[IssueState!]",
+			in: map[string]interface{}{
+				"required": []IssueState{IssueStateOpen, IssueStateClosed},
+				"optional": &[]IssueState{IssueStateOpen, IssueStateClosed},
+			},
+			want: "$optional:[IssueState!]$required:[IssueState!]!",
 		},
 		{
-			in:   map[string]interface{}{"states": []IssueState(nil)},
-			want: "$states:[IssueState!]",
+			in: map[string]interface{}{
+				"required": []IssueState(nil),
+				"optional": (*[]IssueState)(nil),
+			},
+			want: "$optional:[IssueState!]$required:[IssueState!]!",
 		},
 		{
-			in:   map[string]interface{}{"states": [...]IssueState{IssueStateOpen, IssueStateClosed}},
-			want: "$states:[IssueState!]",
+			in: map[string]interface{}{
+				"required": [...]IssueState{IssueStateOpen, IssueStateClosed},
+				"optional": &[...]IssueState{IssueStateOpen, IssueStateClosed},
+			},
+			want: "$optional:[IssueState!]$required:[IssueState!]!",
 		},
 		{
-			in:   map[string]interface{}{"id": ID("someid")},
+			in:   map[string]interface{}{"id": ID("someID")},
 			want: "$id:ID!",
 		},
+		{
+			in:   map[string]interface{}{"ids": []ID{"someID", "anotherID"}},
+			want: `$ids:[ID!]!`,
+		},
+		{
+			in:   map[string]interface{}{"ids": &[]ID{"someID", "anotherID"}},
+			want: `$ids:[ID!]`,
+		},
 	}
-	for _, tc := range tests {
+	for i, tc := range tests {
 		got := queryArguments(tc.in)
 		if got != tc.want {
-			t.Errorf("%s: got: %q, want: %q", tc.name, got, tc.want)
+			t.Errorf("test case %d:\n got: %q\nwant: %q", i, got, tc.want)
 		}
 	}
 }