Repository: googleapis/api-linter
Pull request #382 (2019-11-23 23:57:27)

[Refactor] Unspecified: should check the enum value.
We would like to ensure that a rule checks only the descriptor as provided; in other words, a rule should not return a problem about other descriptors. It makes a rule much easy to understand and for future refactoring. 

For example, we would like to remove `Descriptor` in the `Problem`, as we already know which `Descriptor` a rule is checking and therefore it is redundant to ask the rule developer to return it again in a problem.


Patch:
diff --git a/lint/rule.go b/lint/rule.go
index 9d79724..82aeadb 100644
--- a/lint/rule.go
+++ b/lint/rule.go
@@ -228,7 +228,7 @@ func (r *EnumRule) GetName() RuleName {
 	return r.Name
 }
 
-// Lint visits every service in the file and runs `LintEnum`.
+// Lint visits every enum in the file and runs `LintEnum`.
 //
 // If an `OnlyIf` function is provided on the rule, it is run against each
 // enum, and if it returns false, the `LintEnum` function is not called.
@@ -244,6 +244,43 @@ func (r *EnumRule) Lint(fd *desc.FileDescriptor) []Problem {
 	return problems
 }
 
+// EnumValueRule defines a lint rule that is run on each enum value.
+type EnumValueRule struct {
+	Name RuleName
+
+	// LintEnumValue accepts a EnumValueDescriptor and lints it.
+	LintEnumValue func(*desc.EnumValueDescriptor) []Problem
+
+	// OnlyIf accepts an EnumValueDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.EnumValueDescriptor) bool
+
+	noPositional struct{}
+}
+
+// GetName returns the name of the rule.
+func (r *EnumValueRule) GetName() RuleName {
+	return r.Name
+}
+
+// Lint visits every enum value in the file and runs `LintEnum`.
+//
+// If an `OnlyIf` function is provided on the rule, it is run against each
+// enum value, and if it returns false, the `LintEnum` function is not called.
+func (r *EnumValueRule) Lint(fd *desc.FileDescriptor) []Problem {
+	problems := []Problem{}
+
+	// Lint all enums, either at the top of the file, or nested within messages.
+	for _, enum := range getAllEnums(fd) {
+		for _, value := range enum.GetValues() {
+			if r.OnlyIf == nil || r.OnlyIf(value) {
+				problems = append(problems, r.LintEnumValue(value)...)
+			}
+		}
+	}
+	return problems
+}
+
 // DescriptorRule defines a lint rule that is run on every descriptor
 // in the file (but not the file itself).
 type DescriptorRule struct {
diff --git a/rules/aip0126/unspecified.go b/rules/aip0126/unspecified.go
index eba8b7f..4aae186 100644
--- a/rules/aip0126/unspecified.go
+++ b/rules/aip0126/unspecified.go
@@ -24,20 +24,25 @@ import (
 	"github.com/stoewer/go-strcase"
 )
 
-var unspecified = &lint.EnumRule{
-	Name: lint.NewRuleName(126, "unspecified"),
-	LintEnum: func(e *desc.EnumDescriptor) []lint.Problem {
-		firstValue := e.GetValues()[0]
-		want := strings.ToUpper(strcase.SnakeCase(e.GetName()) + "_UNSPECIFIED")
-		if firstValue.GetName() != want {
+var unspecified = &lint.EnumValueRule{
+	Name:   lint.NewRuleName(126, "unspecified"),
+	OnlyIf: isFirstEnumValue,
+	LintEnumValue: func(v *desc.EnumValueDescriptor) []lint.Problem {
+		want := strings.ToUpper(strcase.SnakeCase(v.GetEnum().GetName()) + "_UNSPECIFIED")
+		if v.GetName() != want {
 			return []lint.Problem{{
 				Message:    fmt.Sprintf("The first enum value should be %q", want),
 				Suggestion: want,
-				Descriptor: firstValue,
-				Location:   locations.DescriptorName(firstValue),
+				Descriptor: v,
+				Location:   locations.DescriptorName(v),
 			}}
 		}
 
 		return nil
 	},
 }
+
+// Note: proto3 requires that first value in enum have numeric value of 0.
+func isFirstEnumValue(v *desc.EnumValueDescriptor) bool {
+	return v.GetNumber() == 0
+}


Test patch:
diff --git a/lint/rule_test.go b/lint/rule_test.go
index 88ed290..c14b429 100644
--- a/lint/rule_test.go
+++ b/lint/rule_test.go
@@ -240,6 +240,34 @@ func TestEnumRule(t *testing.T) {
 	}
 }
 
+func TestEnumValueRule(t *testing.T) {
+	// Create a file descriptor with a top-level enum with values.
+	fd, err := builder.NewFile("test.proto").AddEnum(
+		builder.NewEnum("Format").AddValue(builder.NewEnumValue("YAML")).AddValue(builder.NewEnumValue("JSON")),
+	).Build()
+	if err != nil {
+		t.Fatalf("Catastrophic failure, could not build proto. BOOMZ!")
+	}
+
+	for _, test := range makeLintRuleTests(fd.GetEnumTypes()[0].GetValues()[1]) {
+		t.Run(test.testName, func(t *testing.T) {
+			// Create the enum value rule.
+			rule := &EnumValueRule{
+				Name: RuleName("test"),
+				OnlyIf: func(e *desc.EnumValueDescriptor) bool {
+					return e.GetName() == "JSON"
+				},
+				LintEnumValue: func(e *desc.EnumValueDescriptor) []Problem {
+					return test.problems
+				},
+			}
+
+			// Run the rule and assert that we got what we expect.
+			test.runRule(rule, fd, t)
+		})
+	}
+}
+
 func TestEnumRuleNested(t *testing.T) {
 	// Create a file descriptor with top-level enums.
 	fd, err := builder.NewFile("test.proto").AddMessage(