Repository: googleapis/gapic-generator-go
Pull request #16 (2018-09-26 21:12:27)

sample: properly import client package
Previously we imported the gRPC client, not the GAPIC client


Patch:
diff --git a/cmd/gen-go-sample/inittree.go b/cmd/gen-go-sample/inittree.go
index b5cce54..231ff0b 100644
--- a/cmd/gen-go-sample/inittree.go
+++ b/cmd/gen-go-sample/inittree.go
@@ -208,7 +208,7 @@ func (t *initTree) print(w *bufio.Writer, g *generator, ind int) error {
 	}
 	g.imports[impSpec] = true
 
-	fmt.Fprintf(w, "%s.%s{\n", impSpec.Name, desc.GetName())
+	fmt.Fprintf(w, "&%s.%s{\n", impSpec.Name, desc.GetName())
 	for i, k := range t.keys {
 		for i := 0; i < ind; i++ {
 			w.WriteByte('\t')
diff --git a/cmd/gen-go-sample/main.go b/cmd/gen-go-sample/main.go
index 088d68f..c77adea 100644
--- a/cmd/gen-go-sample/main.go
+++ b/cmd/gen-go-sample/main.go
@@ -38,17 +38,21 @@ import (
 func main() {
 	descFname := flag.String("desc", "", "proto descriptor")
 	gapicFname := flag.String("gapic", "", "gapic config")
+	clientPkg := flag.String("clientpkg", "", "the package of the client, in format 'url/to/client/pkg;name'")
 	nofmt := flag.Bool("nofmt", false, "skip gofmt, useful for debugging code with syntax error")
 	flag.Parse()
 
-	var (
-		gen = generator{
-			imports: map[pbinfo.ImportSpec]bool{},
-		}
+	gen := generator{
+		imports: map[pbinfo.ImportSpec]bool{},
+	}
 
-		donec = make(chan struct{})
-	)
+	if p := strings.IndexByte(*clientPkg, ';'); p >= 0 {
+		gen.clientPkg = pbinfo.ImportSpec{Path: (*clientPkg)[:p], Name: (*clientPkg)[p+1:]}
+	} else {
+		log.Fatalf("need -clientPkg in 'url/to/client/pkg;name' format, got %q", *clientPkg)
+	}
 
+	donec := make(chan struct{})
 	go func() {
 		f, err := os.Open(*gapicFname)
 		if err != nil {
@@ -95,6 +99,7 @@ func main() {
 					}
 
 					gen.reset()
+					gen.imports[gen.clientPkg] = true
 					if err := gen.genSample(iface.Name, meth.Name, sam.RegionTag, vs); err != nil {
 						log.Fatal(errors.E(err, "value set: %q", vs))
 					}
@@ -112,6 +117,8 @@ type generator struct {
 	descInfo pbinfo.Info
 	gapic    GAPICConfig
 
+	clientPkg pbinfo.ImportSpec
+
 	pt      printer.P
 	imports map[pbinfo.ImportSpec]bool
 }
@@ -177,11 +184,6 @@ func (g *generator) genSample(ifaceName, methName, regTag string, valSet SampleV
 		return errors.E(nil, "can't find service: %q", ifaceName)
 	}
 
-	servSpec, err := g.descInfo.ImportSpec(serv)
-	if err != nil {
-		return errors.E(err, "can't import service: %q", ifaceName)
-	}
-
 	var meth *descriptor.MethodDescriptorProto
 	for _, m := range serv.GetMethod() {
 		if m.GetName() == methName {
@@ -198,11 +200,9 @@ func (g *generator) genSample(ifaceName, methName, regTag string, valSet SampleV
 	p("// [START %s]", regTag)
 	p("")
 
-	// TODO(pongad): The package of the client is not the package of the service!
-	// TODO(pongad): properly reduce service name instead of hardcoding "NewClient"
 	p("func sample%s() {", methName)
 	p("  ctx := context.Background()")
-	p("  c := %s.%s(ctx)", servSpec.Name, "NewClient")
+	p("  c := %s.New%sClient(ctx)", g.clientPkg.Name, pbinfo.ReduceServName(serv.GetName(), g.clientPkg.Name))
 	p("")
 	g.imports[pbinfo.ImportSpec{Path: "context"}] = true
 
@@ -257,7 +257,6 @@ func (g *generator) genSample(ifaceName, methName, regTag string, valSet SampleV
 	p("}")
 	p("")
 
-	g.imports[servSpec] = true
 	g.imports[inSpec] = true
 	g.imports[pbinfo.ImportSpec{Path: "fmt"}] = true
 	return nil
diff --git a/internal/gengapic/example.go b/internal/gengapic/example.go
index d5c7d11..9f74c02 100644
--- a/internal/gengapic/example.go
+++ b/internal/gengapic/example.go
@@ -21,7 +21,7 @@ import (
 )
 
 func (g *generator) genExampleFile(serv *descriptor.ServiceDescriptorProto, pkgName string) error {
-	servName := reduceServName(*serv.Name, pkgName)
+	servName := pbinfo.ReduceServName(*serv.Name, pkgName)
 	p := g.printf
 
 	p("func ExampleNew%sClient() {", servName)
diff --git a/internal/gengapic/gengapic.go b/internal/gengapic/gengapic.go
index 9b09759..772051a 100644
--- a/internal/gengapic/gengapic.go
+++ b/internal/gengapic/gengapic.go
@@ -80,7 +80,7 @@ func Gen(genReq *plugin.CodeGeneratorRequest) (*plugin.CodeGeneratorResponse, er
 		// so even though the client for LoggingServiceV2 is just "Client"
 		// the file name is "logging_client.go".
 		// Keep the current behavior for now, but we could revisit this later.
-		outFile := reduceServName(s.GetName(), "")
+		outFile := pbinfo.ReduceServName(s.GetName(), "")
 		outFile = camelToSnake(outFile)
 		outFile = filepath.Join(outDir, outFile)
 
@@ -241,7 +241,7 @@ func (g *generator) reset() {
 
 // gen generates client for the given service.
 func (g *generator) gen(serv *descriptor.ServiceDescriptorProto, pkgName string) error {
-	servName := reduceServName(*serv.Name, pkgName)
+	servName := pbinfo.ReduceServName(*serv.Name, pkgName)
 	if err := g.clientOptions(serv, servName); err != nil {
 		return err
 	}
@@ -436,36 +436,9 @@ func (g *generator) comment(s string) {
 	}
 }
 
-// reduceServName removes redundant components from the service name.
-// For example, FooServiceV2 -> Foo.
-// The returned name is used as part of longer names, like FooClient.
-// If the package name and the service name is the same,
-// reduceServName returns empty string, so we get foo.Client instead of foo.FooClient.
-func reduceServName(svc, pkg string) string {
-	// remove trailing version
-	if p := strings.LastIndexByte(svc, 'V'); p >= 0 {
-		isVer := true
-		for _, r := range svc[p+1:] {
-			if !unicode.IsDigit(r) {
-				isVer = false
-				break
-			}
-		}
-		if isVer {
-			svc = svc[:p]
-		}
-	}
-
-	svc = strings.TrimSuffix(svc, "Service")
-	if strings.EqualFold(svc, pkg) {
-		svc = ""
-	}
-	return svc
-}
-
 // grpcClientField reports the field name to store gRPC client.
 func grpcClientField(reducedServName string) string {
-	// Not the same as reduceServName(*serv.Name, pkg)+"Client".
+	// Not the same as pbinfo.ReduceServName(*serv.Name, pkg)+"Client".
 	// If the service name is reduced to empty string, we should
 	// lower-case "client" so that the field is not exported.
 	return lowerFirst(reducedServName + "Client")
diff --git a/internal/pbinfo/pbinfo.go b/internal/pbinfo/pbinfo.go
index 299dd30..2d481f9 100644
--- a/internal/pbinfo/pbinfo.go
+++ b/internal/pbinfo/pbinfo.go
@@ -18,6 +18,7 @@ package pbinfo
 import (
 	"fmt"
 	"strings"
+	"unicode"
 
 	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/protoc-gen-go/descriptor"
@@ -133,3 +134,30 @@ func (in *Info) ImportSpec(e proto.Message) (ImportSpec, error) {
 		return ImportSpec{Path: pkg, Name: elem + "pb"}, nil
 	}
 }
+
+// ReduceServName removes redundant components from the service name.
+// For example, FooServiceV2 -> Foo.
+// The returned name is used as part of longer names, like FooClient.
+// If the package name and the service name is the same,
+// ReduceServName returns empty string, so we get foo.Client instead of foo.FooClient.
+func ReduceServName(svc, pkg string) string {
+	// remove trailing version
+	if p := strings.LastIndexByte(svc, 'V'); p >= 0 {
+		isVer := true
+		for _, r := range svc[p+1:] {
+			if !unicode.IsDigit(r) {
+				isVer = false
+				break
+			}
+		}
+		if isVer {
+			svc = svc[:p]
+		}
+	}
+
+	svc = strings.TrimSuffix(svc, "Service")
+	if strings.EqualFold(svc, pkg) {
+		svc = ""
+	}
+	return svc
+}


Test patch:
diff --git a/cmd/gen-go-sample/main_test.go b/cmd/gen-go-sample/main_test.go
index fd1cf12..a5ccd6a 100644
--- a/cmd/gen-go-sample/main_test.go
+++ b/cmd/gen-go-sample/main_test.go
@@ -45,6 +45,7 @@ func diff(t *testing.T, name, got, goldenFile string) {
 	}
 }
 
+// TODO(pongad): maybe we should baseline test the whole file.
 func TestSample(t *testing.T) {
 	inType := &descriptor.DescriptorProto{
 		Name: proto.String("InputType"),
@@ -62,7 +63,7 @@ func TestSample(t *testing.T) {
 	}
 
 	serv := &descriptor.ServiceDescriptorProto{
-		Name: proto.String("MyService"),
+		Name: proto.String("FooService"),
 		Method: []*descriptor.MethodDescriptorProto{
 			{
 				Name:      proto.String("MyMethod"),
@@ -72,12 +73,13 @@ func TestSample(t *testing.T) {
 	}
 	file := &descriptor.FileDescriptorProto{
 		Options: &descriptor.FileOptions{
-			GoPackage: proto.String("path/to/mypackage;mypackage"),
+			GoPackage: proto.String("path/to/foo;foo"),
 		},
 	}
 
 	g := generator{
-		imports: map[pbinfo.ImportSpec]bool{},
+		clientPkg: pbinfo.ImportSpec{Path: "path/to/foo", Name: "foo"},
+		imports:   map[pbinfo.ImportSpec]bool{},
 		descInfo: pbinfo.Info{
 			Serv: map[string]*descriptor.ServiceDescriptorProto{
 				".MyService": serv,
diff --git a/cmd/gen-go-sample/test.bash b/cmd/gen-go-sample/test.bash
index 9104ac5..a6fbee1 100755
--- a/cmd/gen-go-sample/test.bash
+++ b/cmd/gen-go-sample/test.bash
@@ -26,5 +26,6 @@ if [ -z $COMMON_PROTO ]; then
 fi
 
 ./gen-go-sample $* \
+  -clientpkg 'cloud.google.com/go/language/apiv1;language' \
   -gapic "$GOOGLEAPIS/google/cloud/language/v1/language_gapic.yaml" \
   -desc <(protoc -o /dev/stdout --include_imports -I "$COMMON_PROTO" -I "$GOOGLEAPIS" "$GOOGLEAPIS"/google/cloud/language/v1/*.proto)
diff --git a/cmd/gen-go-sample/testdata/sample.want b/cmd/gen-go-sample/testdata/sample.want
index 3c97f49..2b570b1 100644
--- a/cmd/gen-go-sample/testdata/sample.want
+++ b/cmd/gen-go-sample/testdata/sample.want
@@ -2,10 +2,10 @@
 
 func sampleMyMethod() {
 	ctx := context.Background()
-	c := mypackagepb.NewClient(ctx)
+	c := foo.NewClient(ctx)
 
-req := mypackagepb.InputType{
-	A: mypackagepb.AType{
+req := &foopb.InputType{
+	A: &foopb.AType{
 		X: 42,
 		Y: 3.14159,
 },
diff --git a/internal/gengapic/gengapic_test.go b/internal/gengapic/gengapic_test.go
index 2cba16e..eeadd09 100644
--- a/internal/gengapic/gengapic_test.go
+++ b/internal/gengapic/gengapic_test.go
@@ -94,8 +94,8 @@ func TestReduceServName(t *testing.T) {
 
 		{"FooV2Bar", "", "FooV2Bar"},
 	} {
-		if got := reduceServName(tst.in, tst.pkg); got != tst.want {
-			t.Errorf("reduceServName(%q, %q) = %q, want %q", tst.in, tst.pkg, got, tst.want)
+		if got := pbinfo.ReduceServName(tst.in, tst.pkg); got != tst.want {
+			t.Errorf("pbinfo.ReduceServName(%q, %q) = %q, want %q", tst.in, tst.pkg, got, tst.want)
 		}
 	}
 }
@@ -110,8 +110,8 @@ func TestGRPCClientField(t *testing.T) {
 		{"FooServiceV2", "foo", "client"},
 		{"FooV2Bar", "", "fooV2BarClient"},
 	} {
-		if got := grpcClientField(reduceServName(tst.in, tst.pkg)); got != tst.want {
-			t.Errorf("grpcClientField(reduceServName(%q, %q)) = %q, want %q", tst.in, tst.pkg, got, tst.want)
+		if got := grpcClientField(pbinfo.ReduceServName(tst.in, tst.pkg)); got != tst.want {
+			t.Errorf("grpcClientField(pbinfo.ReduceServName(%q, %q)) = %q, want %q", tst.in, tst.pkg, got, tst.want)
 		}
 	}
 }