Repository: uber/prototool
Pull request #285 (2019-01-10 06:05:52)

Split up Vim ALE linters
This makes it easier to pick and choose, for example a future linter `prototool-break-check` can be easily combined with `prototool-lint`. The original `prototool` linter is kept around for backwards compatibility.

codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/285?src=pr&el=h1) Report
> Merging [#285](https://codecov.io/gh/uber/prototool/pull/285?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/dd0e69cac812b35549a3afe931133fafba331cc6?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##              dev     #285   +/-   ##
=======================================
  Coverage   72.18%   72.18%           
=======================================
  Files         121      121           
  Lines        5462     5462           
=======================================
  Hits         3943     3943           
  Misses       1141     1141           
  Partials      378      378
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/285?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/285?src=pr&el=footer). Last update [dd0e69c...c7fa789](https://codecov.io/gh/uber/prototool/pull/285?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 5ffcdac..a802de6 100644
--- a/README.md
+++ b/README.md
@@ -419,8 +419,7 @@ Prototool is meant to help enforce a consistent development style for Protobuf,
 
 This repository is a self-contained plugin for use with the [ALE Lint Engine](https://github.com/w0rp/ale). It should be similarly easy to add support for Syntastic, Neomake, etc.
 
-The Vim integration will currently provide lint errors, optionally regenerate all the stubs, and optionally format your files on save. It
-will also optionally create new files from a template when opened.
+The Vim integration will currently compile, provide lint errors, do generation of your stubs, and format your files on save. It will also optionally create new files from a template when opened.
 
 The plugin is under [vim/prototool](vim/prototool), so your plugin manager needs to point there instead of the base of this repository. Assuming you are using [vim-plug](https://github.com/junegunn/vim-plug), copy/paste the following into your vimrc and you should be good to go. If you are using [Vundle](https://github.com/VundleVim/Vundle.vim), just replace `Plug` with `Vundle` below.
 
@@ -438,15 +437,15 @@ Plug 'w0rp/ale'
 Plug 'uber/prototool', { 'rtp':'vim/prototool' }
 
 " We recommend setting just this for Golang, as well as the necessary set for proto.
+" Note the 'prototool' linter is still available, but deprecated in favor of individual linters.
+" Use the 'prototool-compile' linter to just compile, 'prototool-lint' to compile and lint,
+" 'prototool-all' to compile, do generation of your stubs, and then lint.
 let g:ale_linters = {
 \   'go': ['golint'],
-\   'proto': ['prototool'],
+\   'proto': ['prototool-lint'],
 \}
 " We recommend you set this.
 let g:ale_lint_on_text_changed = 'never'
-" Set to 'lint' to not do code generation.
-" Set to 'compile' to not do linting either and just compile without code generation.
-"let g:ale_proto_prototool_command = 'compile'
 
 " We generally have <leader> mapped to ",", uncomment this to set leader.
 "let mapleader=","
@@ -465,6 +464,19 @@ nnoremap <silent> <leader>c :call PrototoolCreateToggle()<CR>
 "call PrototoolCreateDisable()
 ```
 
+The recommended setup in short:
+
+```vim
+Plug 'w0rp/ale'
+Plug 'uber/prototool', { 'rtp':'vim/prototool' }
+let g:ale_linters = {
+\   'go': ['golint'],
+\   'proto': ['prototool-lint'],
+\}
+let g:ale_lint_on_text_changed = 'never'
+call PrototoolFormatFixEnable()
+```
+
 Editor integration is a key goal of Prototool. We've demonstrated support internally for Intellij, and hope that we have integration for more editors in the future.
 
 ## Stability
diff --git a/internal/lint/check_service_names_api_suffix.go b/internal/lint/check_service_names_api_suffix.go
new file mode 100644
index 0000000..d17072e
--- /dev/null
+++ b/internal/lint/check_service_names_api_suffix.go
@@ -0,0 +1,52 @@
+// 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 serviceNamesAPISuffixLinter = NewLinter(
+	"SERVICE_NAMES_API_SUFFIX",
+	`Verifies that all service names end with "API".`,
+	checkServiceNamesAPISuffix,
+)
+
+func checkServiceNamesAPISuffix(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(serviceNamesAPISuffixVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type serviceNamesAPISuffixVisitor struct {
+	baseAddVisitor
+}
+
+func (v serviceNamesAPISuffixVisitor) VisitService(service *proto.Service) {
+	if !strings.HasSuffix(service.Name, "API") {
+		if strings.HasSuffix(service.Name, "Service") {
+			v.AddFailuref(service.Position, `Service name %q must end with "API". Since it currently ends with "Service", this likely means replacing "Service" with "API"`, service.Name)
+		} else {
+			v.AddFailuref(service.Position, `Service name %q must end with "API".`, service.Name)
+		}
+	}
+}
diff --git a/internal/lint/check_wkt_duration_suffix.go b/internal/lint/check_wkt_duration_suffix.go
new file mode 100644
index 0000000..4911f82
--- /dev/null
+++ b/internal/lint/check_wkt_duration_suffix.go
@@ -0,0 +1,68 @@
+// 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 wktDurationSuffixLinter = NewLinter(
+	"WKT_DURATION_SUFFIX",
+	"Verifies that all google.protobuf.Duration field names end in _duration.",
+	checkWKTDurationSuffix,
+)
+
+func checkWKTDurationSuffix(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(wktDurationSuffixVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type wktDurationSuffixVisitor struct {
+	baseAddVisitor
+}
+
+func (v wktDurationSuffixVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v wktDurationSuffixVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v wktDurationSuffixVisitor) VisitNormalField(field *proto.NormalField) {
+	v.visitField(field.Field)
+}
+
+func (v wktDurationSuffixVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.visitField(field.Field)
+}
+
+func (v wktDurationSuffixVisitor) visitField(field *proto.Field) {
+	if field.Type == "google.protobuf.Duration" && !strings.HasSuffix(field.Name, "_duration") {
+		v.AddFailuref(field.Position, `Field %q of type "google.protobuf.Duration" must end in "_duration".`, field.Name)
+	}
+}
diff --git a/internal/lint/check_wkt_timestamp_suffix.go b/internal/lint/check_wkt_timestamp_suffix.go
new file mode 100644
index 0000000..c78a510
--- /dev/null
+++ b/internal/lint/check_wkt_timestamp_suffix.go
@@ -0,0 +1,68 @@
+// 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 wktTimestampSuffixLinter = NewLinter(
+	"WKT_TIMESTAMP_SUFFIX",
+	"Verifies that all google.protobuf.Timestamp field names end in _time.",
+	checkWKTTimestampSuffix,
+)
+
+func checkWKTTimestampSuffix(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(wktTimestampSuffixVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type wktTimestampSuffixVisitor struct {
+	baseAddVisitor
+}
+
+func (v wktTimestampSuffixVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v wktTimestampSuffixVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v wktTimestampSuffixVisitor) VisitNormalField(field *proto.NormalField) {
+	v.visitField(field.Field)
+}
+
+func (v wktTimestampSuffixVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.visitField(field.Field)
+}
+
+func (v wktTimestampSuffixVisitor) visitField(field *proto.Field) {
+	if field.Type == "google.protobuf.Timestamp" && !strings.HasSuffix(field.Name, "_time") {
+		v.AddFailuref(field.Position, `Field %q of type "google.protobuf.Timestamp" must end in "_time".`, field.Name)
+	}
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index e7cbe2e..ca3a786 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -87,10 +87,13 @@ var (
 		requestResponseTypesUniqueLinter,
 		requestResponseNamesMatchRPCLinter,
 		servicesHaveCommentsLinter,
+		serviceNamesAPISuffixLinter,
 		serviceNamesCamelCaseLinter,
 		serviceNamesCapitalizedLinter,
 		syntaxProto3Linter,
 		wktDirectlyImportedLinter,
+		wktDurationSuffixLinter,
+		wktTimestampSuffixLinter,
 	}
 
 	// DefaultLinters is the slice of default Linters.
@@ -189,10 +192,13 @@ var (
 		requestResponseNamesMatchRPCLinter,
 		requestResponseTypesInSameFileLinter,
 		requestResponseTypesUniqueLinter,
+		serviceNamesAPISuffixLinter,
 		serviceNamesCamelCaseLinter,
 		serviceNamesCapitalizedLinter,
 		syntaxProto3Linter,
 		wktDirectlyImportedLinter,
+		wktDurationSuffixLinter,
+		wktTimestampSuffixLinter,
 	}
 
 	// GroupToLinters is the map from linter group to the corresponding slice of linters.
diff --git a/vim/prototool/ale_linters/proto/prototool.vim b/vim/prototool/ale_linters/proto/prototool.vim
index d4053f6..6fc5c48 100644
--- a/vim/prototool/ale_linters/proto/prototool.vim
+++ b/vim/prototool/ale_linters/proto/prototool.vim
@@ -1,4 +1,6 @@
-" Description: run the prototool linter
+" Description: run prototool
+"
+" Deprecated: use prototool-compile, prototool-lint, or prototool-all instead
 
 call ale#Set('proto_prototool_command', 'all')
 
diff --git a/vim/prototool/ale_linters/proto/prototool_all.vim b/vim/prototool/ale_linters/proto/prototool_all.vim
new file mode 100644
index 0000000..0083d80
--- /dev/null
+++ b/vim/prototool/ale_linters/proto/prototool_all.vim
@@ -0,0 +1,14 @@
+" Description: run prototool all
+
+function! ale_linters#proto#prototool_all#GetCommand(buffer) abort
+  return 'prototool all --disable-format %s'
+endfunction
+
+call ale#linter#Define('proto', {
+    \   'name': 'prototool-all',
+    \   'lint_file': 1,
+    \   'output_stream': 'stdout',
+    \   'executable': 'prototool',
+    \   'command_callback': 'ale_linters#proto#prototool_all#GetCommand',
+    \   'callback': 'ale#handlers#unix#HandleAsError',
+    \})
diff --git a/vim/prototool/ale_linters/proto/prototool_compile.vim b/vim/prototool/ale_linters/proto/prototool_compile.vim
new file mode 100644
index 0000000..4f64474
--- /dev/null
+++ b/vim/prototool/ale_linters/proto/prototool_compile.vim
@@ -0,0 +1,14 @@
+" Description: run prototool compile
+
+function! ale_linters#proto#prototool_compile#GetCommand(buffer) abort
+  return 'prototool compile %s'
+endfunction
+
+call ale#linter#Define('proto', {
+    \   'name': 'prototool-compile',
+    \   'lint_file': 1,
+    \   'output_stream': 'stdout',
+    \   'executable': 'prototool',
+    \   'command_callback': 'ale_linters#proto#prototool_compile#GetCommand',
+    \   'callback': 'ale#handlers#unix#HandleAsError',
+    \})
diff --git a/vim/prototool/ale_linters/proto/prototool_lint.vim b/vim/prototool/ale_linters/proto/prototool_lint.vim
new file mode 100644
index 0000000..63d050a
--- /dev/null
+++ b/vim/prototool/ale_linters/proto/prototool_lint.vim
@@ -0,0 +1,14 @@
+" Description: run prototool lint
+
+function! ale_linters#proto#prototool_lint#GetCommand(buffer) abort
+  return 'prototool lint %s'
+endfunction
+
+call ale#linter#Define('proto', {
+    \   'name': 'prototool-lint',
+    \   'lint_file': 1,
+    \   'output_stream': 'stdout',
+    \   'executable': 'prototool',
+    \   'command_callback': 'ale_linters#proto#prototool_lint#GetCommand',
+    \   'callback': 'ale#handlers#unix#HandleAsError',
+    \})


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index 57b964d..e2678fc 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -399,6 +399,14 @@ func TestLint(t *testing.T) {
 		"testdata/lint/noreserved/foo.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`10:1:SERVICE_NAMES_API_SUFFIX
+		12:1:SERVICE_NAMES_API_SUFFIX`,
+		"testdata/lint/apisuffix/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
@@ -448,6 +456,16 @@ func TestLint(t *testing.T) {
 		"testdata/lint/nokeywords/foo/public/public_suppressed.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`14:3:WKT_TIMESTAMP_SUFFIX
+		15:3:WKT_DURATION_SUFFIX
+		17:5:WKT_TIMESTAMP_SUFFIX
+		18:5:WKT_DURATION_SUFFIX`,
+		"testdata/lint/wktsuffix/foo/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/apisuffix/foo.proto b/internal/cmd/testdata/lint/apisuffix/foo.proto
new file mode 100644
index 0000000..94e7f81
--- /dev/null
+++ b/internal/cmd/testdata/lint/apisuffix/foo.proto
@@ -0,0 +1,12 @@
+syntax = "proto3";
+
+package foo;
+
+option go_package = "foopb";
+option java_multiple_files = true;
+option java_outer_classname = "FooProto";
+option java_package = "com.foo";
+
+service HelloService {}
+
+service Hello {}
diff --git a/internal/cmd/testdata/lint/apisuffix/prototool.yaml b/internal/cmd/testdata/lint/apisuffix/prototool.yaml
new file mode 100644
index 0000000..677f7b2
--- /dev/null
+++ b/internal/cmd/testdata/lint/apisuffix/prototool.yaml
@@ -0,0 +1,4 @@
+lint:
+  rules:
+    add:
+      - SERVICE_NAMES_API_SUFFIX
diff --git a/internal/cmd/testdata/lint/wktsuffix/foo/foo.proto b/internal/cmd/testdata/lint/wktsuffix/foo/foo.proto
new file mode 100644
index 0000000..77886b6
--- /dev/null
+++ b/internal/cmd/testdata/lint/wktsuffix/foo/foo.proto
@@ -0,0 +1,20 @@
+syntax = "proto3";
+
+package foo;
+
+option go_package = "foopb";
+option java_multiple_files = true;
+option java_outer_classname = "FooProto";
+option java_package = "com.foo";
+
+import "google/protobuf/duration.proto";
+import "google/protobuf/timestamp.proto";
+
+message Bar {
+  google.protobuf.Timestamp one = 1;
+  google.protobuf.Duration two = 2;
+  oneof test {
+    google.protobuf.Timestamp three = 3;
+    google.protobuf.Duration four = 4;
+  }
+}
diff --git a/internal/cmd/testdata/lint/wktsuffix/prototool.yaml b/internal/cmd/testdata/lint/wktsuffix/prototool.yaml
new file mode 100644
index 0000000..15247af
--- /dev/null
+++ b/internal/cmd/testdata/lint/wktsuffix/prototool.yaml
@@ -0,0 +1,5 @@
+lint:
+  rules:
+    add:
+      - WKT_DURATION_SUFFIX
+      - WKT_TIMESTAMP_SUFFIX