Repository: uber/prototool
Pull request #294 (2019-01-11 23:55:39)

Add linter to check that service names match the file name
This makes sure that each file:

- Only has one service
- That the file name is `service_name_lower_snake_case.proto`

This also adds a test for #293.

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

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

```diff
@@            Coverage Diff            @@
##              dev     #294     +/-   ##
=========================================
+ Coverage   72.68%   72.79%   +0.1%     
=========================================
  Files         126      127      +1     
  Lines        5697     5720     +23     
=========================================
+ Hits         4141     4164     +23     
- Misses       1167     1168      +1     
+ Partials      389      388      -1
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/294?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/294/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `79.2% <ø> (ø)` | :arrow_up: |
| [internal/strs/strs.go](https://codecov.io/gh/uber/prototool/pull/294/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc3Rycy9zdHJzLmdv) | `100% <100%> (ø)` | :arrow_up: |
| [...ternal/lint/check\_service\_names\_match\_file\_name.go](https://codecov.io/gh/uber/prototool/pull/294/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19zZXJ2aWNlX25hbWVzX21hdGNoX2ZpbGVfbmFtZS5nbw==) | `80.95% <80.95%> (ø)` | |
| [.../lint/check\_request\_response\_types\_only\_in\_file.go](https://codecov.io/gh/uber/prototool/pull/294/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19yZXF1ZXN0X3Jlc3BvbnNlX3R5cGVzX29ubHlfaW5fZmlsZS5nbw==) | `94.87% <0%> (+10.25%)` | :arrow_up: |

------

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


Patch:
diff --git a/internal/lint/check_service_names_match_file_name.go b/internal/lint/check_service_names_match_file_name.go
new file mode 100644
index 0000000..5733ebc
--- /dev/null
+++ b/internal/lint/check_service_names_match_file_name.go
@@ -0,0 +1,74 @@
+// 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 (
+	"path/filepath"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/strs"
+	"github.com/uber/prototool/internal/text"
+)
+
+var serviceNamesMatchFileNameLinter = NewLinter(
+	"SERVICE_NAMES_MATCH_FILE_NAME",
+	"Verifies that there is one service per file and the file name is service_name_lower_snake_case.proto.",
+	checkServiceNamesMatchFileName,
+)
+
+func checkServiceNamesMatchFileName(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(&serviceNamesMatchFileNameVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type serviceNamesMatchFileNameVisitor struct {
+	baseAddVisitor
+	filename string
+	services []*proto.Service
+}
+
+func (v *serviceNamesMatchFileNameVisitor) OnStart(descriptor *proto.Proto) error {
+	v.filename = descriptor.Filename
+	v.services = nil
+	return nil
+}
+
+func (v *serviceNamesMatchFileNameVisitor) VisitService(service *proto.Service) {
+	v.services = append(v.services, service)
+}
+
+func (v *serviceNamesMatchFileNameVisitor) Finally() error {
+	if len(v.services) == 0 {
+		return nil
+	}
+	if len(v.services) > 1 {
+		for _, service := range v.services {
+			v.AddFailuref(service.Position, `Multiple services defined in this file and there should be only one service per file.`)
+		}
+		return nil
+	}
+	service := v.services[0]
+	filename := filepath.Base(v.filename)
+	expectedFilename := strs.ToLowerSnakeCase(service.Name) + ".proto"
+	if filename != expectedFilename {
+		v.AddFailuref(service.Position, `Expected filename to be %q for file containing service %q but was %q.`, expectedFilename, service.Name, filename)
+	}
+	return nil
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index 99d16f2..3dbcd9c 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -100,6 +100,7 @@ var (
 		serviceNamesAPISuffixLinter,
 		serviceNamesCamelCaseLinter,
 		serviceNamesCapitalizedLinter,
+		serviceNamesMatchFileNameLinter,
 		syntaxProto3Linter,
 		wktDirectlyImportedLinter,
 		wktDurationSuffixLinter,
@@ -215,6 +216,7 @@ var (
 		serviceNamesAPISuffixLinter,
 		serviceNamesCamelCaseLinter,
 		serviceNamesCapitalizedLinter,
+		serviceNamesMatchFileNameLinter,
 		syntaxProto3Linter,
 		wktDirectlyImportedLinter,
 		wktDurationSuffixLinter,
diff --git a/internal/strs/strs.go b/internal/strs/strs.go
index f108f5b..100cb47 100644
--- a/internal/strs/strs.go
+++ b/internal/strs/strs.go
@@ -85,6 +85,11 @@ func IsUpperSnakeCase(s string) bool {
 	return true
 }
 
+// ToLowerSnakeCase converts s to lower_snake_case.
+func ToLowerSnakeCase(s string) string {
+	return strings.ToLower(toSnake(s))
+}
+
 // ToUpperSnakeCase converts s to UPPER_SNAKE_CASE.
 func ToUpperSnakeCase(s string) string {
 	return strings.ToUpper(toSnake(s))


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index daccbcc..2ab5864 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -443,6 +443,29 @@ func TestLint(t *testing.T) {
 		"testdata/lint/floats/foo/v1/foo.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`23:1:REQUEST_RESPONSE_TYPES_ONLY_IN_FILE
+		27:1:REQUEST_RESPONSE_TYPES_ONLY_IN_FILE`,
+		"testdata/lint/onlyinfile/foo/v1/hello_api.proto",
+	)
+
+	assertDoLintFile(
+		t,
+		false,
+		`13:1:SERVICE_NAMES_MATCH_FILE_NAME
+		15:1:SERVICE_NAMES_MATCH_FILE_NAME`,
+		"testdata/lint/servicenamefilename/foo/v1/another_api.proto",
+	)
+
+	assertDoLintFile(
+		t,
+		false,
+		`13:1:SERVICE_NAMES_MATCH_FILE_NAME`,
+		"testdata/lint/servicenamefilename/foo/v1/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/onlyinfile/foo/v1/hello_api.proto b/internal/cmd/testdata/lint/onlyinfile/foo/v1/hello_api.proto
new file mode 100644
index 0000000..1f0cb19
--- /dev/null
+++ b/internal/cmd/testdata/lint/onlyinfile/foo/v1/hello_api.proto
@@ -0,0 +1,29 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option csharp_namespace = "Foo.V1";
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "HelloApiProto";
+option java_package = "com.foo.v1";
+option objc_class_prefix = "FXX";
+option php_namespace = "Foo\\V1";
+
+service HelloAPI {
+  rpc SendFoo(SendFooRequest) returns (SendFooResponse);
+}
+
+message SendFooRequest {
+  Foo foo = 1;
+}
+
+message SendFooResponse {}
+
+message Foo {
+  message NestedFoo {}
+}
+
+enum Bar {
+  BAR_INVALID = 0;
+}
diff --git a/internal/cmd/testdata/lint/onlyinfile/prototool.yaml b/internal/cmd/testdata/lint/onlyinfile/prototool.yaml
new file mode 100644
index 0000000..092dbf5
--- /dev/null
+++ b/internal/cmd/testdata/lint/onlyinfile/prototool.yaml
@@ -0,0 +1,2 @@
+lint:
+  group: uber2
diff --git a/internal/cmd/testdata/lint/servicenamefilename/foo/v1/another_api.proto b/internal/cmd/testdata/lint/servicenamefilename/foo/v1/another_api.proto
new file mode 100644
index 0000000..abd6b86
--- /dev/null
+++ b/internal/cmd/testdata/lint/servicenamefilename/foo/v1/another_api.proto
@@ -0,0 +1,15 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option csharp_namespace = "Foo.V1";
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "AnotherApiProto";
+option java_package = "com.foo.v1";
+option objc_class_prefix = "FXX";
+option php_namespace = "Foo\\V1";
+
+service AnotherAPI {}
+
+service EvenAnotherAPI {}
diff --git a/internal/cmd/testdata/lint/servicenamefilename/foo/v1/foo.proto b/internal/cmd/testdata/lint/servicenamefilename/foo/v1/foo.proto
new file mode 100644
index 0000000..9941467
--- /dev/null
+++ b/internal/cmd/testdata/lint/servicenamefilename/foo/v1/foo.proto
@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option csharp_namespace = "Foo.V1";
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "FooProto";
+option java_package = "com.foo.v1";
+option objc_class_prefix = "FXX";
+option php_namespace = "Foo\\V1";
+
+service HelloAPI {}
diff --git a/internal/cmd/testdata/lint/servicenamefilename/prototool.yaml b/internal/cmd/testdata/lint/servicenamefilename/prototool.yaml
new file mode 100644
index 0000000..092dbf5
--- /dev/null
+++ b/internal/cmd/testdata/lint/servicenamefilename/prototool.yaml
@@ -0,0 +1,2 @@
+lint:
+  group: uber2
diff --git a/internal/strs/strs_test.go b/internal/strs/strs_test.go
index f3933f1..4a5030c 100644
--- a/internal/strs/strs_test.go
+++ b/internal/strs/strs_test.go
@@ -82,6 +82,17 @@ func TestIsUppercase(t *testing.T) {
 	assert.True(t, IsUppercase("HELLO"))
 }
 
+func TestToLowerSnakeCase(t *testing.T) {
+	assert.Equal(t, "", ToUpperSnakeCase(""))
+	assert.Equal(t, "camel_case", ToLowerSnakeCase("CamelCase"))
+	assert.Equal(t, "camel_case", ToLowerSnakeCase("camelCase"))
+	assert.Equal(t, "camel_case_", ToLowerSnakeCase("CamelCase_"))
+	assert.Equal(t, "_camel_case", ToLowerSnakeCase("_CamelCase"))
+	assert.Equal(t, "camel_case__hello", ToLowerSnakeCase("CamelCase__Hello"))
+	assert.Equal(t, "abbr_camel", ToLowerSnakeCase("ABBRCamel"))
+	assert.Equal(t, "foo_abbr_camel", ToLowerSnakeCase("FooABBRCamel"))
+}
+
 func TestToUpperSnakeCase(t *testing.T) {
 	assert.Equal(t, "", ToUpperSnakeCase(""))
 	assert.Equal(t, "CAMEL_CASE", ToUpperSnakeCase("CamelCase"))