Repository: googleapis/api-linter
Pull request #374 (2019-11-21 19:27:55)

[AIP-191] Enforce ruby_package idiomaticity.

lukesneeringer: > Just want to double-check that Google::UpperCamelExample::V1 is accepted. (Because Google::UpperCamelExample::V1Beta is correctly not accepted, but that is valid upper-camel, and I don't see code that distinguishes that case.) Can you add a test case real quick?

It is accepted (we checked a variant of this in #369), but I am happy to add a test.

One corner case that is _not_ accepted is `Google::Cloud::AutoML::V1`, which it will try to change to `AutoMl`. I think we just have to live with that (we have the ability to disable the rule on an as-needed basis).
dazuma: Oh. Okay, well, `AutoML` is an uncommon case in Ruby so I guess we can disable the rule in that case.
lukesneeringer: > Oh. Okay, well, `AutoML` is an uncommon case in Ruby so I guess we can disable the rule in that case.

I added a note about it to the documentation, too.

Patch:
diff --git a/docs/rules/0191/csharp-namespace.md b/docs/rules/0191/csharp-namespace.md
index 937eb8c..e86ab5b 100644
--- a/docs/rules/0191/csharp-namespace.md
+++ b/docs/rules/0191/csharp-namespace.md
@@ -82,6 +82,12 @@ package google.example.v1beta1;
 option csharp_namespace = "Google.Example.V1Beta1";
 ```
 
+## Known issues
+
+This rule will improperly complain if it encounters an acronym. For example, it
+will complain about `Google.Cloud.AutoML.V1`, preferring `AutoMl`. This lint
+rule **should** be disabled in this case.
+
 ## Disabling
 
 If you need to violate this rule, use a comment at the top of the file.
diff --git a/docs/rules/0191/ruby-package.md b/docs/rules/0191/ruby-package.md
new file mode 100644
index 0000000..e8c8d8f
--- /dev/null
+++ b/docs/rules/0191/ruby-package.md
@@ -0,0 +1,107 @@
+---
+rule:
+  aip: 191
+  name: [core, '0191', ruby-package]
+  summary: The `option ruby_package` annotation should be idiomatic if set.
+permalink: /191/ruby-package
+redirect_from:
+  - /0191/ruby-package
+---
+
+# Ruby package annotation
+
+This rule enforces that if a proto file for a public API surface sets
+`option ruby_package`, that it uses language idiomatic conventions, as mandated
+in [AIP-191][].
+
+## Details
+
+This rule looks at each proto file, and complains if the `ruby_package` file
+annotation uses anything other than upper camel case, or includes characters
+other than letters, numbers, and `:`.
+
+It also ensures that versions with stability (e.g. `V1beta1`) are capitalized
+appropriately.
+
+## Examples
+
+### Case
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1;
+
+option ruby_package = "google::example::v1";  // Should be UpperCamelCase.
+```
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1;
+
+option ruby_package = "Google.Example.V1";  // Separator should be `::`.
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+syntax = "proto3";
+
+package google.example.v1;
+
+option ruby_package = "Google::Example::V1";
+```
+
+### Versions with stability
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1beta1;
+
+option ruby_package = "Google::Example::V1Beta1"; // Should be V1beta1.
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+syntax = "proto3";
+
+package google.example.v1beta1;
+
+option ruby_package = "Google::Example::V1beta1";
+```
+
+## Known issues
+
+This rule will improperly complain if it encounters an acronym. For example, it
+will complain about `Google::Cloud::AutoML::V1`, preferring `AutoMl`. This lint
+rule **should** be disabled in this case.
+
+## Disabling
+
+If you need to violate this rule, use a comment at the top of the file.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0191::ruby-package=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+syntax = "proto3";
+
+package google.example.v1;
+
+option ruby_package = "google::example::v1";
+```
+
+[aip-191]: https://aip.dev/191
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/locations/file_locations.go b/locations/file_locations.go
index d1d82e6..664107e 100644
--- a/locations/file_locations.go
+++ b/locations/file_locations.go
@@ -43,3 +43,12 @@ func FilePackage(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
 func FileCsharpNamespace(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
 	return pathLocation(f, 8, 37) // 8 == options, 37 == csharp_namespace
 }
+
+// FileRubyPackage returns the location of the ruby_package file option
+// in a file descriptor.
+//
+// If the location can not be found (for example, because there is no
+// ruby_package option), it returns nil.
+func FileRubyPackage(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
+	return pathLocation(f, 8, 45) // 8 == options, 45 == ruby_package
+}
diff --git a/rules/aip0191/aip0191.go b/rules/aip0191/aip0191.go
index 5156e2e..8b0ebb1 100644
--- a/rules/aip0191/aip0191.go
+++ b/rules/aip0191/aip0191.go
@@ -31,6 +31,7 @@ func AddRules(r lint.RuleRegistry) error {
 		javaMultipleFiles,
 		javaOuterClassname,
 		javaPackage,
+		rubyPackage,
 		syntax,
 	)
 }
diff --git a/rules/aip0191/ruby_package.go b/rules/aip0191/ruby_package.go
new file mode 100644
index 0000000..c978045
--- /dev/null
+++ b/rules/aip0191/ruby_package.go
@@ -0,0 +1,59 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0191
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/locations"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/stoewer/go-strcase"
+)
+
+var rubyPackage = &lint.FileRule{
+	Name: lint.NewRuleName(191, "ruby-package"),
+	LintFile: func(f *desc.FileDescriptor) []lint.Problem {
+		if ns := f.GetFileOptions().GetRubyPackage(); ns != "" {
+			// Check for invalid characters.
+			if !rubyValidChars.MatchString(ns) {
+				return []lint.Problem{{
+					Message:    "Invalid characters: Ruby packages only allow [A-Za-z0-9:].",
+					Descriptor: f,
+					Location:   locations.FileRubyPackage(f),
+				}}
+			}
+
+			// Check that upper camel case is used.
+			upperCamel := []string{}
+			for _, segment := range strings.Split(ns, "::") {
+				upperCamel = append(upperCamel, strcase.UpperCamelCase(segment))
+			}
+			if want := strings.Join(upperCamel, "::"); ns != want {
+				return []lint.Problem{{
+					Message:    "Ruby packages use UpperCamelCase.",
+					Suggestion: fmt.Sprintf("option ruby_package = %q;", want),
+					Descriptor: f,
+					Location:   locations.FileRubyPackage(f),
+				}}
+			}
+		}
+		return nil
+	},
+}
+
+var rubyValidChars = regexp.MustCompile("^[A-Za-z0-9:]+$")


Test patch:
diff --git a/locations/file_locations_test.go b/locations/file_locations_test.go
index 80992ce..f0e9beb 100644
--- a/locations/file_locations_test.go
+++ b/locations/file_locations_test.go
@@ -31,6 +31,7 @@ func TestLocations(t *testing.T) {
 		package google.api.linter;
 
 		option csharp_namespace = "Google.Api.Linter";
+		option ruby_package = "Google::Api::Linter";
 
 		message Foo {
 			string bar = 1;
@@ -47,6 +48,7 @@ func TestLocations(t *testing.T) {
 			{"Syntax", FileSyntax, []int32{1, 0, int32(len("syntax = \"proto3\";"))}},
 			{"Package", FilePackage, []int32{3, 0, int32(len("package google.api.linter;"))}},
 			{"CsharpNamespace", FileCsharpNamespace, []int32{5, 0, int32(len(`option csharp_namespace = "Google.Api.Linter";`))}},
+			{"RubyPackage", FileRubyPackage, []int32{6, 0, int32(len(`option ruby_package = "Google::Api::Linter";`))}},
 		}
 		for _, test := range tests {
 			t.Run(test.testName, func(t *testing.T) {
diff --git a/rules/aip0191/ruby_package_test.go b/rules/aip0191/ruby_package_test.go
new file mode 100644
index 0000000..e487c2d
--- /dev/null
+++ b/rules/aip0191/ruby_package_test.go
@@ -0,0 +1,55 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0191
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestRubyPackage(t *testing.T) {
+	for _, test := range []struct {
+		name        string
+		RubyPackage string
+		problems    testutils.Problems
+	}{
+		{"Valid", "Google::Example::V1", testutils.Problems{}},
+		{"ValidBeta", "Google::Example::V1beta1", testutils.Problems{}},
+		{"ValidTwoWords", "Google::LibraryExample::V1", testutils.Problems{}},
+		{"InvalidBadChars", "Google.Example.V1", testutils.Problems{{Message: "Invalid characters"}}},
+		{"Invalid", "google::example::v1", testutils.Problems{{
+			Suggestion: fmt.Sprintf("option ruby_package = %q;", "Google::Example::V1"),
+		}}},
+		{"InvalidVersion", "Google::Example::v1", testutils.Problems{{
+			Suggestion: fmt.Sprintf("option ruby_package = %q;", "Google::Example::V1"),
+		}}},
+		{"InvalidBeta", "Google::Example::V1Beta1", testutils.Problems{{
+			Suggestion: fmt.Sprintf("option ruby_package = %q;", "Google::Example::V1beta1"),
+		}}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				package google.example.v1;
+
+				option ruby_package = "{{.RubyPackage}}";
+			`, test)
+			if diff := test.problems.SetDescriptor(f).Diff(rubyPackage.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}