Support package-names with dashes in them (#232)

Support package-names with dashes in them

We were smart about aliasing if you have name-collisions, but not if
your package name is something that's not a valid identifier, like
`"path/to/my-package"`, which Go for better or worse allows. Now we
remove all the invalid characters (in practice mainly dashes, dots, and
leading digits).

Fixes #231.

Test plan: make check

Author: benjaminjkraft

docs/CHANGELOG.md

@@ -28,7 +28,8 @@ When releasing a new version:
 
 ### Bug fixes:
 
-- Fixed non-deterministic generated code when querying graphql interfaces
+- Fixed non-deterministic generated code when querying graphql interfaces.
+- Fixed generated code when last component of package name is not a valid identifier (e.g. `"path/to/my-package"`).
 
 ## v0.5.0
 

generate/generate_test.go

@@ -171,6 +171,10 @@ func TestGenerateWithConfig(t *testing.T) {
 			Generated:   "generated.go",
 			ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
 		}},
+		{"CustomContextWithAlias", "", nil, &Config{
+			Generated:   "generated.go",
+			ContextType: "github.com/Khan/genqlient/internal/testutil/junk---fun.name.MyContext",
+		}},
 		{"StructReferences", "", nil, &Config{
 			StructReferences: true,
 			Generated:        "generated-structrefs.go",

generate/imports.go

@@ -2,14 +2,43 @@ package generate
 
 import (
 	"fmt"
+	"go/token"
 	"go/types"
 	"regexp"
 	"strconv"
 	"strings"
+	"unicode"
 )
 
+// makeIdentifier takes a string and returns a valid go identifier like it.
+//
+// If the string is an identifier, return the input. Otherwise, munge it to
+// make a valid identifier, which at worst (if the input is entirely emoji,
+// say) means coming up with one out of whole cloth. This identifier need not
+// be particularly unique; the caller may add a suffix.
+func makeIdentifier(candidateIdentifier string) string {
+	if token.IsIdentifier(candidateIdentifier) {
+		return candidateIdentifier
+	}
+
+	var goodChars strings.Builder
+	for _, c := range candidateIdentifier {
+		// modified from token.IsIdentifier
+		if unicode.IsLetter(c) || c == '_' ||
+			// digits only valid after first char
+			goodChars.Len() > 0 && unicode.IsDigit(c) {
+			goodChars.WriteRune(c)
+		}
+	}
+	if goodChars.Len() > 0 {
+		return goodChars.String()
+	}
+
+	return "alias"
+}
+
 func (g *generator) addImportFor(pkgPath string) (alias string) {
-	pkgName := pkgPath[strings.LastIndex(pkgPath, "/")+1:]
+	pkgName := makeIdentifier(pkgPath[strings.LastIndex(pkgPath, "/")+1:])
 	alias = pkgName
 	suffix := 2
 	for g.usedAliases[alias] {

generate/imports_test.go

@@ -0,0 +1,42 @@
+package generate
+
+import (
+	"go/token"
+	"testing"
+)
+
+func TestMakeIdentifier(t *testing.T) {
+	tests := []struct {
+		testName string
+		input    string
+		expected string
+	}{
+		{"GoodIdentifier", "myIdent", "myIdent"},
+		{"GoodIdentifierNumbers", "myIdent1234", "myIdent1234"},
+		{"NumberPrefix", "1234myIdent", "myIdent"},
+		{"OnlyNumbers", "1234", "alias"},
+		{"Dashes", "my-ident", "myident"},
+		// Note: most Go implementations won't actually allow
+		// this package-path, but the spec is pretty vague
+		// so make sure to handle it.
+		{"JunkAnd", "my!!\\\\\nident", "myident"},
+		{"JunkOnly", "!!\\\\\n", "alias"},
+		{"Accents", "née", "née"},
+		{"Kanji", "日本", "日本"},
+		{"EmojiAnd", "ident👍", "ident"},
+		{"EmojiOnly", "👍", "alias"},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.testName, func(t *testing.T) {
+			actual := makeIdentifier(test.input)
+			if actual != test.expected {
+				t.Errorf("mismatch:\ngot:  %s\nwant: %s", actual, test.expected)
+			}
+			if !token.IsIdentifier(actual) {
+				t.Errorf("not a valid identifier: %s", actual)
+			}
+		})
+	}
+}

generate/testdata/snapshots/TestGenerateWithConfig-CustomContextWithAlias-testdata-queries-generated.go

@@ -0,0 +1,51 @@
+package testutil
+
+import (
+	"context"
+	"time"
+
+	"github.com/Khan/genqlient/graphql"
+)
+
+type ID string
+
+type Pokemon struct {
+	Species string `json:"species"`
+	Level   int    `json:"level"`
+}
+
+func (p Pokemon) Battle(q Pokemon) bool {
+	return p.Level > q.Level
+}
+
+type MyContext interface {
+	context.Context
+
+	MyMethod()
+}
+
+func GetClientFromNowhere() (graphql.Client, error)                    { return nil, nil }
+func GetClientFromContext(ctx context.Context) (graphql.Client, error) { return nil, nil }
+func GetClientFromMyContext(ctx MyContext) (graphql.Client, error)     { return nil, nil }
+
+const dateFormat = "2006-01-02"
+
+func MarshalDate(t *time.Time) ([]byte, error) {
+	// nil should never happen but we might as well check.  zero-time does
+	// happen because omitempty doesn't consider it zero; we'd prefer to write
+	// null than "0001-01-01".
+	//
+	// (I mean, we're tests.  Who cares!  But we may as well try to match what
+	// prod code would want.)
+	if t == nil || t.IsZero() {
+		return []byte("null"), nil
+	}
+	return []byte(`"` + t.Format(dateFormat) + `"`), nil
+}
+
+func UnmarshalDate(b []byte, t *time.Time) error {
+	// (modified from time.Time.UnmarshalJSON)
+	var err error
+	*t, err = time.Parse(`"`+dateFormat+`"`, string(b))
+	return err
+}