Repository: uber/prototool
Pull request #319 (2019-01-30 17:32:34)

More linters
Re: #301.

- Lint for `filepath` instead of `file_path`
- Lint that all non-map fields that contain `time` are `google.protobuf.Timestamps`
- Lint t hat all non-map fields that contain `duration` are `google.protobuf.Durations` 

codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=h1) Report
> Merging [#319](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/1d2421f4a66b45eef9334b72ea2e51e38f51148c?src=pr&el=desc) will **increase** coverage by `0.23%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/uber/prototool/pull/319/graphs/tree.svg?width=650&token=A9V0DApqZg&height=150&src=pr)](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             dev     #319      +/-   ##
=========================================
+ Coverage   73.4%   73.63%   +0.23%     
=========================================
  Files        141      143       +2     
  Lines       6305     6316      +11     
=========================================
+ Hits        4628     4651      +23     
+ Misses      1254     1246       -8     
+ Partials     423      419       -4
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `71.01% <ø> (ø)` | :arrow_up: |
| [...nternal/lint/check\_message\_field\_names\_filepath.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19tZXNzYWdlX2ZpZWxkX25hbWVzX2ZpbGVwYXRoLmdv) | `100% <100%> (ø)` | |
| [internal/lint/check\_message\_fields\_duration.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19tZXNzYWdlX2ZpZWxkc19kdXJhdGlvbi5nbw==) | `100% <100%> (ø)` | |
| [internal/lint/check\_message\_fields\_time.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19tZXNzYWdlX2ZpZWxkc190aW1lLmdv) | `100% <100%> (ø)` | |
| [internal/breaking/breaking.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYnJlYWtpbmcvYnJlYWtpbmcuZ28=) | `72.72% <0%> (ø)` | :arrow_up: |
| [...ternal/breaking/check\_message\_fields\_same\_oneof.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYnJlYWtpbmcvY2hlY2tfbWVzc2FnZV9maWVsZHNfc2FtZV9vbmVvZi5nbw==) | | |
| [internal/extract/extract.go](https://codecov.io/gh/uber/prototool/pull/319/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZXh0cmFjdC9leHRyYWN0Lmdv) | `89.69% <0%> (+4.88%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=footer). Last update [1d2421f...268e2ac](https://codecov.io/gh/uber/prototool/pull/319?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/internal/lint/BUILD.bazel b/internal/lint/BUILD.bazel
index 481c995..fc166fe 100644
--- a/internal/lint/BUILD.bazel
+++ b/internal/lint/BUILD.bazel
@@ -30,10 +30,13 @@ go_library(
         "check_imports_not_public.go",
         "check_imports_not_weak.go",
         "check_message_field_names_filename.go",
+        "check_message_field_names_filepath.go",
         "check_message_field_names_lower_snake_case.go",
         "check_message_field_names_lowercase.go",
+        "check_message_fields_duration.go",
         "check_message_fields_no_json_name.go",
         "check_message_fields_not_floats.go",
+        "check_message_fields_time.go",
         "check_message_names_camel_case.go",
         "check_message_names_capitalized.go",
         "check_messages_have_comments.go",
diff --git a/internal/lint/check_message_field_names_filepath.go b/internal/lint/check_message_field_names_filepath.go
new file mode 100644
index 0000000..2555826
--- /dev/null
+++ b/internal/lint/check_message_field_names_filepath.go
@@ -0,0 +1,73 @@
+// Copyright (c) 2019 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package lint
+
+import (
+	"strings"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+var messageFieldNamesFilepathLinter = NewLinter(
+	"MESSAGE_FIELD_NAMES_FILEPATH",
+	`Verifies that all message field names do not contain "file_path" as "filepath" should be used instead.`,
+	checkMessageFieldNamesFilepath,
+)
+
+func checkMessageFieldNamesFilepath(add func(*text.Failure), dirPath string, descriptors []*FileDescriptor) error {
+	return runVisitor(messageFieldNamesFilepathVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type messageFieldNamesFilepathVisitor struct {
+	baseAddVisitor
+}
+
+func (v messageFieldNamesFilepathVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldNamesFilepathVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldNamesFilepathVisitor) VisitNormalField(field *proto.NormalField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesFilepathVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesFilepathVisitor) VisitMapField(field *proto.MapField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesFilepathVisitor) handleField(field *proto.Field) {
+	if strings.Contains(field.Name, "file_path") {
+		// This might break down in practice but let's try it out
+		v.AddFailuref(field.Position, "Field name %q should be %q.", field.Name, strings.Replace(field.Name, "file_path", "filepath", -1))
+	}
+}
diff --git a/internal/lint/check_message_fields_duration.go b/internal/lint/check_message_fields_duration.go
new file mode 100644
index 0000000..782d57b
--- /dev/null
+++ b/internal/lint/check_message_fields_duration.go
@@ -0,0 +1,70 @@
+// Copyright (c) 2019 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package lint
+
+import (
+	"strings"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+var messageFieldsDurationLinter = NewLinter(
+	"MESSAGE_FIELDS_DURATION",
+	`Verifies that all non-map fields that contain "duration" in their name are google.protobuf.Durations.`,
+	checkMessageFieldsDuration,
+)
+
+func checkMessageFieldsDuration(add func(*text.Failure), dirPath string, descriptors []*FileDescriptor) error {
+	return runVisitor(messageFieldsDurationVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type messageFieldsDurationVisitor struct {
+	baseAddVisitor
+}
+
+func (v messageFieldsDurationVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldsDurationVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldsDurationVisitor) VisitNormalField(field *proto.NormalField) {
+	v.visitField(field.Field)
+}
+
+func (v messageFieldsDurationVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.visitField(field.Field)
+}
+
+// no map fields on purpose
+
+func (v messageFieldsDurationVisitor) visitField(field *proto.Field) {
+	if strings.Contains(field.Name, "duration") && field.Type != "google.protobuf.Duration" {
+		v.AddFailuref(field.Position, `Field %q must be a google.protobuf.Duration.`, field.Name)
+	}
+}
diff --git a/internal/lint/check_message_fields_time.go b/internal/lint/check_message_fields_time.go
new file mode 100644
index 0000000..2794a87
--- /dev/null
+++ b/internal/lint/check_message_fields_time.go
@@ -0,0 +1,70 @@
+// Copyright (c) 2019 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package lint
+
+import (
+	"strings"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+var messageFieldsTimeLinter = NewLinter(
+	"MESSAGE_FIELDS_TIME",
+	`Verifies that all non-map fields that contain "time" in their name are google.protobuf.Timestamps.`,
+	checkMessageFieldsTime,
+)
+
+func checkMessageFieldsTime(add func(*text.Failure), dirPath string, descriptors []*FileDescriptor) error {
+	return runVisitor(messageFieldsTimeVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type messageFieldsTimeVisitor struct {
+	baseAddVisitor
+}
+
+func (v messageFieldsTimeVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldsTimeVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldsTimeVisitor) VisitNormalField(field *proto.NormalField) {
+	v.visitField(field.Field)
+}
+
+func (v messageFieldsTimeVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.visitField(field.Field)
+}
+
+// no map fields on purpose
+
+func (v messageFieldsTimeVisitor) visitField(field *proto.Field) {
+	if strings.Contains(field.Name, "time") && field.Type != "google.protobuf.Timestamp" {
+		v.AddFailuref(field.Position, `Field %q must be a google.protobuf.Timestamp.`, field.Name)
+	}
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index a562d25..1c98dba 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -81,9 +81,12 @@ var (
 		gogoNotImportedLinter,
 		importsNotPublicLinter,
 		importsNotWeakLinter,
+		messageFieldsDurationLinter,
 		messageFieldsNotFloatsLinter,
 		messageFieldsNoJSONNameLinter,
+		messageFieldsTimeLinter,
 		messageFieldNamesFilenameLinter,
+		messageFieldNamesFilepathLinter,
 		messageFieldNamesLowerSnakeCaseLinter,
 		messageFieldNamesLowercaseLinter,
 		messageNamesCamelCaseLinter,
@@ -222,9 +225,12 @@ var (
 		messagesHaveSentenceCommentsExceptRequestResponseTypesLinter,
 		messagesNotEmptyExceptRequestResponseTypesLinter,
 		messageFieldNamesFilenameLinter,
+		messageFieldNamesFilepathLinter,
 		messageFieldNamesLowerSnakeCaseLinter,
+		messageFieldsDurationLinter,
 		messageFieldsNoJSONNameLinter,
 		messageFieldsNotFloatsLinter,
+		messageFieldsTimeLinter,
 		messageNamesCamelCaseLinter,
 		messageNamesCapitalizedLinter,
 		namesNoCommonLinter,


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index 2764329..5b0a1a1 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -499,10 +499,29 @@ func TestLint(t *testing.T) {
 		18:9:MESSAGE_FIELD_NAMES_FILENAME
 		21:5:MESSAGE_FIELD_NAMES_FILENAME
 		23:3:MESSAGE_FIELD_NAMES_FILENAME
-		24:3:MESSAGE_FIELD_NAMES_FILENAME`,
+		24:3:MESSAGE_FIELD_NAMES_FILENAME
+		30:7:MESSAGE_FIELD_NAMES_FILEPATH
+		32:9:MESSAGE_FIELD_NAMES_FILEPATH
+		35:5:MESSAGE_FIELD_NAMES_FILEPATH
+		37:3:MESSAGE_FIELD_NAMES_FILEPATH
+		38:3:MESSAGE_FIELD_NAMES_FILEPATH`,
 		"testdata/lint/fieldnamesfilename/hello.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`16:7:MESSAGE_FIELDS_TIME
+		18:9:MESSAGE_FIELDS_TIME
+		21:5:MESSAGE_FIELDS_TIME
+		23:3:MESSAGE_FIELDS_TIME
+		30:7:MESSAGE_FIELDS_DURATION
+		32:9:MESSAGE_FIELDS_DURATION
+		35:5:MESSAGE_FIELDS_DURATION
+		37:3:MESSAGE_FIELDS_DURATION`,
+		"testdata/lint/fieldstimeduration/hello.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/fieldnamesfilename/hello.proto b/internal/cmd/testdata/lint/fieldnamesfilename/hello.proto
index 2e8bcc5..4f4df11 100644
--- a/internal/cmd/testdata/lint/fieldnamesfilename/hello.proto
+++ b/internal/cmd/testdata/lint/fieldnamesfilename/hello.proto
@@ -23,3 +23,17 @@ message Foo {
   string file_names = 1;
   map<string, int64> map_file_name = 2;
 }
+
+message Bar {
+  message NestedBar {
+    message NestedNestedBar {
+      string sfile_path = 1;
+      oneof hello {
+        string file_path = 2;
+      }
+    }
+    string sfile_paths = 1;
+  }
+  string file_paths = 1;
+  map<string, int64> map_file_path = 2;
+}
diff --git a/internal/cmd/testdata/lint/fieldstimeduration/hello.proto b/internal/cmd/testdata/lint/fieldstimeduration/hello.proto
new file mode 100644
index 0000000..f88fe19
--- /dev/null
+++ b/internal/cmd/testdata/lint/fieldstimeduration/hello.proto
@@ -0,0 +1,39 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option csharp_namespace = "Foo.V1";
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "HelloProto";
+option java_package = "com.foo.v1";
+option objc_class_prefix = "FXX";
+option php_namespace = "Foo\\V1";
+
+message Foo {
+  message NestedFoo {
+    message NestedNestedFoo {
+      string stime = 1;
+      oneof hello {
+        string time = 2;
+      }
+    }
+    string stimes = 1;
+  }
+  string times = 1;
+  map<string, int64> map_time = 2;
+}
+
+message Bar {
+  message NestedBar {
+    message NestedNestedBar {
+      string sfile_duration = 1;
+      oneof hello {
+        string file_duration = 2;
+      }
+    }
+    string sfile_durations = 1;
+  }
+  string file_durations = 1;
+  map<string, int64> map_file_duration = 2;
+}
diff --git a/internal/cmd/testdata/lint/fieldstimeduration/prototool.yaml b/internal/cmd/testdata/lint/fieldstimeduration/prototool.yaml
new file mode 100644
index 0000000..ee80b43
--- /dev/null
+++ b/internal/cmd/testdata/lint/fieldstimeduration/prototool.yaml
@@ -0,0 +1,8 @@
+lint:
+  group: uber2
+  rules:
+    remove:
+      - ENUMS_HAVE_SENTENCE_COMMENTS
+      - MESSAGES_HAVE_SENTENCE_COMMENTS_EXCEPT_REQUEST_RESPONSE_TYPES
+      - RPCS_HAVE_SENTENCE_COMMENTS
+      - SERVICES_HAVE_SENTENCE_COMMENTS