Repository: googleapis/api-linter
Pull request #393 (2019-11-26 21:37:48)

[refactor] Improve utils.GetTypeName
Having different methods is duplicative. One is better.


Patch:
diff --git a/rules/aip0132/request_field_types.go b/rules/aip0132/request_field_types.go
index 3888f41..259986c 100644
--- a/rules/aip0132/request_field_types.go
+++ b/rules/aip0132/request_field_types.go
@@ -33,7 +33,7 @@ var requestFieldTypes = &lint.FieldRule{
 	},
 	LintField: func(f *desc.FieldDescriptor) (problems []lint.Problem) {
 		// Establish that the field being checked is a string.
-		if utils.GetScalarTypeName(f) != "string" {
+		if utils.GetTypeName(f) != "string" {
 			return []lint.Problem{{
 				Message:    fmt.Sprintf("Field %q should be a string.", f.GetName()),
 				Descriptor: f,
diff --git a/rules/aip0141/forbidden_types.go b/rules/aip0141/forbidden_types.go
index d1485da..c867c3c 100644
--- a/rules/aip0141/forbidden_types.go
+++ b/rules/aip0141/forbidden_types.go
@@ -17,29 +17,18 @@ package aip0141
 import (
 	"fmt"
 
-	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
+	"bitbucket.org/creachadair/stringset"
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
 	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
-	"github.com/jhump/protoreflect/desc/builder"
 )
 
 var forbiddenTypes = &lint.FieldRule{
 	Name: lint.NewRuleName(141, "forbidden-types"),
 	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		// Make a map of the forbidden types.
-		nope := make(map[dpb.FieldDescriptorProto_Type]string)
-		for _, t := range []*builder.FieldType{
-			builder.FieldTypeFixed32(),
-			builder.FieldTypeFixed64(),
-			builder.FieldTypeUInt32(),
-			builder.FieldTypeUInt64(),
-		} {
-			// Change "TYPE_TYPENAME" to "typename".
-			nope[t.GetType()] = utils.GetScalarTypeName(t)
-		}
-		if typeName, ok := nope[f.GetType()]; ok {
+		nope := stringset.New("fixed32", "fixed64", "uint32", "uint64")
+		if typeName := utils.GetTypeName(f); nope.Contains(typeName) {
 			// Preserve original intent w/r/t 32-bit vs. 64-bit.
 			want := "int" + typeName[len(typeName)-2:]
 			return []lint.Problem{{
diff --git a/rules/aip0142/time_field_names.go b/rules/aip0142/time_field_names.go
index aa1f231..c7eb16e 100644
--- a/rules/aip0142/time_field_names.go
+++ b/rules/aip0142/time_field_names.go
@@ -45,7 +45,7 @@ var fieldNames = &lint.FieldRule{
 		}
 
 		// Look for timestamps that do not end in `_time`.
-		if utils.GetMessageTypeName(f) == "google.protobuf.Timestamp" && !strings.HasSuffix(f.GetName(), "_time") {
+		if utils.GetTypeName(f) == "google.protobuf.Timestamp" && !strings.HasSuffix(f.GetName(), "_time") {
 			return []lint.Problem{{
 				Message:    "Timestamp fields should end in `_time`.",
 				Descriptor: f,
diff --git a/rules/aip0142/time_field_type.go b/rules/aip0142/time_field_type.go
index ae61ded..64e559b 100644
--- a/rules/aip0142/time_field_type.go
+++ b/rules/aip0142/time_field_type.go
@@ -33,7 +33,7 @@ var fieldType = &lint.FieldRule{
 			"usec", "usecs",
 		)
 		tokens := strings.Split(f.GetName(), "_")
-		if suffixes.Contains(tokens[len(tokens)-1]) && utils.GetMessageTypeName(f) != "google.protobuf.Timestamp" {
+		if suffixes.Contains(tokens[len(tokens)-1]) && utils.GetTypeName(f) != "google.protobuf.Timestamp" {
 			return []lint.Problem{{
 				Message:    "Fields representing timestamps should use `google.protobuf.Timestamp`.",
 				Suggestion: "google.protobuf.Timestamp",
diff --git a/rules/aip0143/string_type.go b/rules/aip0143/string_type.go
index 2263631..3d67e21 100644
--- a/rules/aip0143/string_type.go
+++ b/rules/aip0143/string_type.go
@@ -36,7 +36,7 @@ var fieldTypes = &lint.FieldRule{
 		).Contains(f.GetName())
 	},
 	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		if typeName := utils.GetScalarTypeName(f); typeName != "string" {
+		if typeName := utils.GetTypeName(f); typeName != "string" {
 			return []lint.Problem{{
 				Message:    fmt.Sprintf("Field %q should be a string, not %s.", f.GetName(), typeName),
 				Suggestion: "string",
diff --git a/rules/aip0217/unreachable_field_type.go b/rules/aip0217/unreachable_field_type.go
index 7d83065..b36e424 100644
--- a/rules/aip0217/unreachable_field_type.go
+++ b/rules/aip0217/unreachable_field_type.go
@@ -33,7 +33,7 @@ var unreachableFieldType = &lint.FieldRule{
 				Descriptor: f,
 			})
 		}
-		if utils.GetScalarTypeName(f) != "string" {
+		if utils.GetTypeName(f) != "string" {
 			problems = append(problems, lint.Problem{
 				Message:    "unreachable field should be a string.",
 				Suggestion: "string",
diff --git a/rules/internal/utils/type_name.go b/rules/internal/utils/type_name.go
index 1fbd7f0..fcc3b11 100644
--- a/rules/internal/utils/type_name.go
+++ b/rules/internal/utils/type_name.go
@@ -17,23 +17,19 @@ package utils
 import (
 	"strings"
 
-	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/jhump/protoreflect/desc"
 )
 
-type hasGetType interface {
-	GetType() dpb.FieldDescriptorProto_Type
-}
-
-// GetScalarTypeName returns the name of the type of the field, as a string.
-func GetScalarTypeName(t hasGetType) string {
-	return strings.ToLower(t.GetType().String()[len("TYPE_"):])
-}
-
-// GetMessageTypeName returns the name of the type of the field, as a string.
-func GetMessageTypeName(f *desc.FieldDescriptor) string {
-	if messageType := f.GetMessageType(); messageType != nil {
-		return messageType.GetFullyQualifiedName()
+// GetTypeName returns the name of the type of the field, as a string,
+// regardless
+//
+// TODO: Add support for map types.
+func GetTypeName(f *desc.FieldDescriptor) string {
+	if m := f.GetMessageType(); m != nil {
+		return m.GetFullyQualifiedName()
+	}
+	if e := f.GetEnumType(); e != nil {
+		return e.GetFullyQualifiedName()
 	}
-	return ""
+	return strings.ToLower(f.GetType().String()[len("TYPE_"):])
 }


Test patch:
diff --git a/rules/internal/utils/type_name_test.go b/rules/internal/utils/type_name_test.go
index d233f04..b3f3a0f 100644
--- a/rules/internal/utils/type_name_test.go
+++ b/rules/internal/utils/type_name_test.go
@@ -20,43 +20,22 @@ import (
 	"github.com/googleapis/api-linter/rules/internal/testutils"
 )
 
-func TestGetScalarTypeName(t *testing.T) {
-	for _, ty := range []string{"int32", "int64", "string", "bytes", "float", "double"} {
+func TestGetTypeName(t *testing.T) {
+	for _, ty := range []string{"int32", "int64", "string", "bytes", "google.protobuf.Timestamp", "Format"} {
 		t.Run(ty, func(t *testing.T) {
 			file := testutils.ParseProto3Tmpl(t, `
+				import "google/protobuf/timestamp.proto";
 				message Book {
 					{{.Type}} field = 1;
 				}
+				enum Format {
+					FORMAT_UNSPECIFIED = 0;
+				}
 			`, struct{ Type string }{ty})
 			field := file.GetMessageTypes()[0].GetFields()[0]
-			if got, want := GetScalarTypeName(field), ty; got != want {
+			if got, want := GetTypeName(field), ty; got != want {
 				t.Errorf("Got %q, expected %q.", got, want)
 			}
 		})
 	}
 }
-
-func TestGetMessageTypeName(t *testing.T) {
-	tests := []struct {
-		testName string
-		Type     string
-		want     string
-	}{
-		{"Message", "google.protobuf.Timestamp", "google.protobuf.Timestamp"},
-		{"Scalar", "int32", ""},
-	}
-	for _, test := range tests {
-		t.Run(test.testName, func(t *testing.T) {
-			file := testutils.ParseProto3Tmpl(t, `
-				import "google/protobuf/timestamp.proto";
-				message Book {
-					{{.Type}} field = 1;
-				}
-			`, test)
-			field := file.GetMessageTypes()[0].GetFields()[0]
-			if got := GetMessageTypeName(field); got != test.want {
-				t.Errorf("Got %q, expected %q.", got, test.want)
-			}
-		})
-	}
-}