Repository: uber/prototool
Pull request #276 (2019-01-08 18:03:19)

@suppresswarnings for linters
This adds the concept of `@suppresswarnings` for key linters. See the documentation. The only linter this is enabled for is `MESSAGE_FIELDS_NOT_FLOATS`, which is only part of our new `uber2` lint group.


Patch:
diff --git a/README.md b/README.md
index a0927e9..1ee4b9e 100644
--- a/README.md
+++ b/README.md
@@ -173,6 +173,8 @@ Lint rules can be set using the configuration file. See the configuration at [et
 
 - `google`: This lint group follows the Style Guide at https://developers.google.com/protocol-buffers/docs/style. This is a small group of rules meant to enforce basic naming, and is widely followed. The style guide is copied to [etc/style/google/google.proto](etc/style/google/google.proto).
 - `uber`: This lint group follows the Style Guide at [etc/style/uber/uber.proto](etc/style/uber/uber.proto). This is a very strict rule group and is meant to enforce consistent development patterns.
+- `uber2`: This lint group is the v2 of the `uber` lint group, and makes some modifcations to more closely follow the Google Cloud APIs file
+  structure, as well as adding even more rules to enforce more consistent development patterns. This lint group is under development.
 
 Configuration of your group can be done by setting the `lint.group` option in your `prototool.yaml` file:
 
@@ -186,6 +188,24 @@ See the `prototool.yaml` files at [etc/style/google/prototool.yaml](etc/style/go
 
 The `uber` lint group represents the default lint group, and will be used if no lint group is configured.
 
+While Prototool takes the stance that linting should be a pass/fail exercise, some lint rules are linting for usages that should not be
+commonly used, but still have validity within the lint group construct. These rules can be suppressed by adding `@suppresswarnings ANNOTATION`
+to the element comment. The following lint rules can be suppressed:
+
+
+| Lint Rule                   | Containing Lint Groups   | Suppressing Annotation   |
+|-----------------------------|--------------------------|--------------------------|
+| `MESSAGE_FIELDS_NOT_FLOATS` | `uber2`                  | `floats`                 |
+
+As an example:
+
+```
+// Hello is a field where we understand the concerns with using doubles but require them.
+//
+// @suppresswarnings floats
+double hello = 1;
+```
+
 See [internal/cmd/testdata/lint](internal/cmd/testdata/lint) for additional examples of configurations, and run `prototool lint internal/cmd/testdata/lint/DIR` from a checkout of this repository to see example failures.
 
 Files must be valid Protobuf that can be compiled with `protoc`, so prior to linting, `prototool lint` will compile your using `protoc`.
diff --git a/internal/lint/check_message_fields_not_floats.go b/internal/lint/check_message_fields_not_floats.go
index 6c1f73d..787ae5f 100644
--- a/internal/lint/check_message_fields_not_floats.go
+++ b/internal/lint/check_message_fields_not_floats.go
@@ -21,13 +21,17 @@
 package lint
 
 import (
+	"fmt"
+
 	"github.com/emicklei/proto"
 	"github.com/uber/prototool/internal/text"
 )
 
+const messageFieldsNotFloatsSuppressableAnnotation = "floats"
+
 var messageFieldsNotFloatsLinter = NewLinter(
 	"MESSAGE_FIELDS_NOT_FLOATS",
-	"Verifies that all message fields are not floats or doubles.",
+	fmt.Sprintf(`Suppressable with "@suppresswarnings %s". Verifies that all message fields are not floats or doubles.`, messageFieldsNotFloatsSuppressableAnnotation),
 	checkMessageFieldsNotFloats,
 )
 
@@ -66,6 +70,9 @@ func (v messageFieldsNotFloatsVisitor) VisitMapField(field *proto.MapField) {
 func (v messageFieldsNotFloatsVisitor) checkNotFloat(field *proto.Field) {
 	switch field.Type {
 	case "double", "float":
-		v.AddFailuref(field.Position, "Field %q is a float and floating point types are not allowed, consider using an int64 while representing your value in micros or nanos.", field.Name)
+		if isSuppressed(field.Comment, messageFieldsNotFloatsSuppressableAnnotation) {
+			return
+		}
+		v.AddFailuref(field.Position, `Field %q is a float and floating point types should generally not be used, consider using an int64 while representing your value in micros or nanos. This can be suppressed by adding "@suppresswarnings %s" to the field comment.`, field.Name, messageFieldsNotFloatsSuppressableAnnotation)
 	}
 }
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index 9f35c89..442bfd0 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/emicklei/proto"
 	"github.com/uber/prototool/internal/file"
@@ -173,6 +174,7 @@ var (
 		importsNotWeakLinter,
 		messageFieldNamesLowerSnakeCaseLinter,
 		messageFieldsNoJSONNameLinter,
+		messageFieldsNotFloatsLinter,
 		messageNamesCamelCaseLinter,
 		messageNamesCapitalizedLinter,
 		oneofNamesLowerSnakeCaseLinter,
@@ -390,3 +392,16 @@ func shouldIgnore(linter Linter, descriptor *proto.Proto, ignoreIDToFilePaths ma
 	}
 	return false, nil
 }
+
+func isSuppressed(comment *proto.Comment, annotation string) bool {
+	if comment == nil {
+		return false
+	}
+	annotation = "@suppresswarnings " + annotation
+	for _, line := range comment.Lines {
+		if strings.Contains(line, annotation) {
+			return true
+		}
+	}
+	return false
+}


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index 865d602..bad4716 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -427,6 +427,14 @@ func TestLint(t *testing.T) {
 		"testdata/lint/majorbetaversioned/foo/foo.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`17:3:MESSAGE_FIELDS_NOT_FLOATS
+		18:3:MESSAGE_FIELDS_NOT_FLOATS`,
+		"testdata/lint/floats/foo/v1/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/floats/foo/v1/foo.proto b/internal/cmd/testdata/lint/floats/foo/v1/foo.proto
new file mode 100644
index 0000000..0c0ef63
--- /dev/null
+++ b/internal/cmd/testdata/lint/floats/foo/v1/foo.proto
@@ -0,0 +1,19 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "FooProto";
+option java_package = "com.foo.v1";
+
+message One {
+  // This is a field we are ok with having floats.
+  //
+  // @suppresswarnings floats
+  double one = 1;
+  // @suppresswarnings floats
+  float two = 2;
+  double three = 3;
+  double four = 4;
+}
diff --git a/internal/cmd/testdata/lint/floats/prototool.yaml b/internal/cmd/testdata/lint/floats/prototool.yaml
new file mode 100644
index 0000000..092dbf5
--- /dev/null
+++ b/internal/cmd/testdata/lint/floats/prototool.yaml
@@ -0,0 +1,2 @@
+lint:
+  group: uber2