fix: Make the required and optional rules less noisy. (#760)

This commit makes those lint rules stop complaining if *both* the terms "required" and "optional" are present; this is a common occurrence when fields are conditionally required, which is a good reason to use the terms in comments without an annotation. Fixes #759.
googleapis · Feb 17, 2021 · f27c257 · f27c257
1 parent e253419
commit f27c257
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 34 deletions.
diff --git a/rules/aip0203/aip0203.go b/rules/aip0203/aip0203.go
@@ -42,15 +42,22 @@ func AddRules(r lint.RuleRegistry) error {
 
 // check leading comments of a field and produce a problem
 // if the comments match the give pattern.
-func checkLeadingComments(f *desc.FieldDescriptor, pattern *regexp.Regexp, annotation string) []lint.Problem {
-	leadingComments := f.GetSourceInfo().GetLeadingComments()
-	if pattern.MatchString(leadingComments) {
-		return []lint.Problem{{
-			Message:    fmt.Sprintf("Use the `google.api.field_behavior` annotation instead of %q in the leading comments. For example, `string name = 1 [(google.api.field_behavior) = %s];`.", pattern.FindString(leadingComments), annotation),
-			Descriptor: f,
-		}}
+func checkLeadingComments(pattern *regexp.Regexp, annotation string, unless ...*regexp.Regexp) func(*desc.FieldDescriptor) []lint.Problem {
+	return func(f *desc.FieldDescriptor) []lint.Problem {
+		leadingComments := f.GetSourceInfo().GetLeadingComments()
+		for _, ul := range unless {
+			if ul.MatchString(leadingComments) {
+				return nil
+			}
+		}
+		if pattern.MatchString(leadingComments) {
+			return []lint.Problem{{
+				Message:    fmt.Sprintf("Use the `google.api.field_behavior` annotation instead of %q in the leading comments. For example, `string name = 1 [(google.api.field_behavior) = %s];`.", pattern.FindString(leadingComments), annotation),
+				Descriptor: f,
+			}}
+		}
+		return nil
 	}
-	return nil
 }
 
 func withoutFieldBehavior(f *desc.FieldDescriptor) bool {

diff --git a/rules/aip0203/immutable.go b/rules/aip0203/immutable.go
@@ -23,11 +23,9 @@ import (
 )
 
 var immutable = &lint.FieldRule{
-	Name:   lint.NewRuleName(203, "immutable"),
-	OnlyIf: withoutImmutableFieldBehavior,
-	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		return checkLeadingComments(f, immutableRegexp, "IMMUTABLE")
-	},
+	Name:      lint.NewRuleName(203, "immutable"),
+	OnlyIf:    withoutImmutableFieldBehavior,
+	LintField: checkLeadingComments(immutableRegexp, "IMMUTABLE"),
 }
 
 var immutableRegexp = regexp.MustCompile("(?i).*immutable.*")

diff --git a/rules/aip0203/input_only.go b/rules/aip0203/input_only.go
@@ -23,11 +23,9 @@ import (
 )
 
 var inputOnly = &lint.FieldRule{
-	Name:   lint.NewRuleName(203, "input-only"),
-	OnlyIf: withoutInputOnlyFieldBehavior,
-	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		return checkLeadingComments(f, inputOnlyRegexp, "INPUT_ONLY")
-	},
+	Name:      lint.NewRuleName(203, "input-only"),
+	OnlyIf:    withoutInputOnlyFieldBehavior,
+	LintField: checkLeadingComments(inputOnlyRegexp, "INPUT_ONLY"),
 }
 
 var inputOnlyRegexp = regexp.MustCompile("(?i).*input.?only.*")

diff --git a/rules/aip0203/optional.go b/rules/aip0203/optional.go
@@ -23,11 +23,9 @@ import (
 )
 
 var optional = &lint.FieldRule{
-	Name:   lint.NewRuleName(203, "optional"),
-	OnlyIf: withoutFieldBehavior,
-	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		return checkLeadingComments(f, optionalRegexp, "OPTIONAL")
-	},
+	Name:      lint.NewRuleName(203, "optional"),
+	OnlyIf:    withoutFieldBehavior,
+	LintField: checkLeadingComments(optionalRegexp, "OPTIONAL", requiredRegexp),
 }
 
 var optionalRegexp = regexp.MustCompile("(?i).*optional.*")

diff --git a/rules/aip0203/optional_test.go b/rules/aip0203/optional_test.go
@@ -50,6 +50,12 @@ func TestOptional(t *testing.T) {
 				Message: "google.api.field_behavior",
 			}},
 		},
+		{
+			name:     "valid-required-and-optional",
+			comment:  "optional required",
+			field:    titleField,
+			problems: nil,
+		},
 		{
 			name:    "Invalid-Optional",
 			comment: "Optional",

diff --git a/rules/aip0203/output_only.go b/rules/aip0203/output_only.go
@@ -23,11 +23,9 @@ import (
 )
 
 var outputOnly = &lint.FieldRule{
-	Name:   lint.NewRuleName(203, "output-only"),
-	OnlyIf: withoutOutputOnlyFieldBehavior,
-	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		return checkLeadingComments(f, outputOnlyRegexp, "OUTPUT_ONLY")
-	},
+	Name:      lint.NewRuleName(203, "output-only"),
+	OnlyIf:    withoutOutputOnlyFieldBehavior,
+	LintField: checkLeadingComments(outputOnlyRegexp, "OUTPUT_ONLY"),
 }
 
 var outputOnlyRegexp = regexp.MustCompile("(?i).*output.?only.*")

diff --git a/rules/aip0203/required.go b/rules/aip0203/required.go
@@ -18,15 +18,12 @@ import (
 	"regexp"
 
 	"github.com/googleapis/api-linter/lint"
-	"github.com/jhump/protoreflect/desc"
 )
 
 var required = &lint.FieldRule{
-	Name:   lint.NewRuleName(203, "required"),
-	OnlyIf: withoutFieldBehavior,
-	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
-		return checkLeadingComments(f, requiredRegexp, "REQUIRED")
-	},
+	Name:      lint.NewRuleName(203, "required"),
+	OnlyIf:    withoutFieldBehavior,
+	LintField: checkLeadingComments(requiredRegexp, "REQUIRED", optionalRegexp),
 }
 
 var requiredRegexp = regexp.MustCompile("(?i).*required.*")