Repository: uber/prototool
Pull request #273 (2019-01-07 16:41:20)

Update PACKAGE_MAJOR_VERSIONED to PACKAGE_MAJOR_BETA_VERSIONED
This allows packages to be not just of the form `foo.v1` but also of the form `foo.v1beta1`. Also removes the branches designation from the `.travis.yml` file as this seems flaky.

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

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

```diff
@@            Coverage Diff            @@
##             dev     #273      +/-   ##
=========================================
+ Coverage   71.1%   71.19%   +0.08%     
=========================================
  Files         98       98              
  Lines       4797     4818      +21     
=========================================
+ Hits        3411     3430      +19     
- Misses      1028     1030       +2     
  Partials     358      358
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/273?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/273/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `78.49% <ø> (ø)` | :arrow_up: |
| [...nternal/lint/check\_package\_major\_beta\_versioned.go](https://codecov.io/gh/uber/prototool/pull/273/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19wYWNrYWdlX21ham9yX2JldGFfdmVyc2lvbmVkLmdv) | `66.66% <100%> (ø)` | |
| [internal/protostrs/protostrs.go](https://codecov.io/gh/uber/prototool/pull/273/diff?src=pr&el=tree#diff-aW50ZXJuYWwvcHJvdG9zdHJzL3Byb3Rvc3Rycy5nbw==) | `86.44% <80%> (-3.04%)` | :arrow_down: |

------

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


Patch:
diff --git a/.travis.yml b/.travis.yml
index 849123e..f07fd34 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,7 +1,3 @@
-branches:
-  only:
-    - dev
-    - master
 language: go
 go:
   - "1.11.x"
diff --git a/internal/lint/check_package_major_versioned.go b/internal/lint/check_package_major_beta_versioned.go
similarity index 68%
rename from internal/lint/check_package_major_versioned.go
rename to internal/lint/check_package_major_beta_versioned.go
index ed39161..125aa94 100644
--- a/internal/lint/check_package_major_versioned.go
+++ b/internal/lint/check_package_major_beta_versioned.go
@@ -28,30 +28,30 @@ import (
 	"github.com/uber/prototool/internal/text"
 )
 
-var packageMajorVersionedLinter = NewLinter(
-	"PACKAGE_MAJOR_VERSIONED",
-	`Verifies that the package is of the form "package.vMAJORVERSION".`,
-	checkPackageMajorVersioned,
+var packageMajorBetaVersionedLinter = NewLinter(
+	"PACKAGE_MAJOR_BETA_VERSIONED",
+	`Verifies that the package is of the form "package.vMAJORVERSION" or "package.vMAJORVERSIONbetaBETAVERSION" with versions >=1.`,
+	checkPackageMajorBetaVersioned,
 )
 
-func checkPackageMajorVersioned(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
-	return runVisitor(&packageMajorVersionedVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+func checkPackageMajorBetaVersioned(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(&packageMajorBetaVersionedVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
 }
 
-type packageMajorVersionedVisitor struct {
+type packageMajorBetaVersionedVisitor struct {
 	baseAddVisitor
 
 	filename string
 	pkg      *proto.Package
 }
 
-func (v *packageMajorVersionedVisitor) OnStart(descriptor *proto.Proto) error {
+func (v *packageMajorBetaVersionedVisitor) OnStart(descriptor *proto.Proto) error {
 	v.filename = descriptor.Filename
 	v.pkg = nil
 	return nil
 }
 
-func (v *packageMajorVersionedVisitor) VisitPackage(pkg *proto.Package) {
+func (v *packageMajorBetaVersionedVisitor) VisitPackage(pkg *proto.Package) {
 	if v.pkg != nil {
 		v.AddFailuref(pkg.Position, "multiple package declarations, first was %v", v.pkg)
 		return
@@ -59,13 +59,13 @@ func (v *packageMajorVersionedVisitor) VisitPackage(pkg *proto.Package) {
 	v.pkg = pkg
 }
 
-func (v *packageMajorVersionedVisitor) Finally() error {
+func (v *packageMajorBetaVersionedVisitor) Finally() error {
 	if v.pkg == nil {
 		v.AddFailuref(scanner.Position{Filename: v.filename}, "No package declaration found.")
 		return nil
 	}
-	if _, ok := protostrs.MajorVersion(v.pkg.Name); !ok {
-		v.AddFailuref(v.pkg.Position, `Package should be of the form "package.vMAJORVERSION" but was %q.`, v.pkg.Name)
+	if _, _, ok := protostrs.MajorBetaVersion(v.pkg.Name); !ok {
+		v.AddFailuref(v.pkg.Position, `Package should be of the form "package.vMAJORVERSION" or "packagevMAJORVERSIONbetaBETAVERSION" with versions >=1 but was %q.`, v.pkg.Name)
 	}
 	return nil
 }
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index 7668b96..37fae0c 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -76,7 +76,7 @@ var (
 		oneofNamesLowerSnakeCaseLinter,
 		packageIsDeclaredLinter,
 		packageLowerSnakeCaseLinter,
-		packageMajorVersionedLinter,
+		packageMajorBetaVersionedLinter,
 		packagesSameInDirLinter,
 		rpcsHaveCommentsLinter,
 		rpcNamesCamelCaseLinter,
diff --git a/internal/protostrs/protostrs.go b/internal/protostrs/protostrs.go
index 7682424..14db663 100644
--- a/internal/protostrs/protostrs.go
+++ b/internal/protostrs/protostrs.go
@@ -85,31 +85,62 @@ func JavaPackage(packageName string) string {
 	return "com." + packageName
 }
 
-// MajorVersion extracts the major version number from the package name, if
-// present. A package must be of the form "foo.vMAJORVERSION". Returns the
-// version and true if the package is of this form, 0 and false otherwise.
-func MajorVersion(packageName string) (uint64, bool) {
+// MajorBetaVersion extracts the major and beta version number from the package
+// name, if present. A package must be of the form "foo.vMAJORVERSION" or
+// "foo.vMAJORVERSIONbetaBETAVERSION" . Returns the major version, beta version
+// and true if the package is of this form, 0 and false otherwise. If there is
+// no beta version, 0 is returned. Valid versions are >=1.
+func MajorBetaVersion(packageName string) (uint64, uint64, bool) {
 	if packageName == "" {
-		return 0, false
+		return 0, 0, false
 	}
 	split := strings.Split(packageName, ".")
 	// A package named "vX" should not count as it is just a single package name,
 	// not a package name and a version.
 	if len(split) < 2 {
-		return 0, false
+		return 0, 0, false
 	}
+
 	versionPart := split[len(split)-1]
 	// Must be 'v' along with at least one number.
 	if len(versionPart) < 2 {
-		return 0, false
+		return 0, 0, false
 	}
 	if versionPart[0] != 'v' {
-		return 0, false
+		return 0, 0, false
 	}
+
 	versionString := versionPart[1:]
-	version, err := strconv.ParseUint(versionString, 10, 64)
-	if err != nil {
-		return 0, false
+	versionStringSplit := strings.Split(versionString, "beta")
+	switch len(versionStringSplit) {
+	case 1:
+		version, err := strconv.ParseUint(versionString, 10, 64)
+		if err != nil {
+			return 0, 0, false
+		}
+		if version == 0 {
+			return 0, 0, false
+		}
+		return version, 0, true
+	case 2:
+		majorVersionString := versionStringSplit[0]
+		betaVersionString := versionStringSplit[1]
+		if majorVersionString == "" || betaVersionString == "" {
+			return 0, 0, false
+		}
+		majorVersion, err := strconv.ParseUint(majorVersionString, 10, 64)
+		if err != nil {
+			return 0, 0, false
+		}
+		betaVersion, err := strconv.ParseUint(betaVersionString, 10, 64)
+		if err != nil {
+			return 0, 0, false
+		}
+		if majorVersion == 0 || betaVersion == 0 {
+			return 0, 0, false
+		}
+		return majorVersion, betaVersion, true
+	default:
+		return 0, 0, false
 	}
-	return version, true
 }
diff --git a/internal/reflect/proto/prototool.yaml b/internal/reflect/proto/prototool.yaml
index 21032fc..d768f32 100644
--- a/internal/reflect/proto/prototool.yaml
+++ b/internal/reflect/proto/prototool.yaml
@@ -13,7 +13,7 @@ lint:
       - IMPORTS_NOT_WEAK
       - MESSAGE_FIELDS_NO_JSON_NAME
       - MESSAGES_HAVE_COMMENTS_EXCEPT_REQUEST_RESPONSE_TYPES
-      - PACKAGE_MAJOR_VERSIONED
+      - PACKAGE_MAJOR_BETA_VERSIONED
       - REQUEST_RESPONSE_NAMES_MATCH_RPC
       - RPCS_HAVE_COMMENTS
     remove:


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index ce96574..39da084 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -420,6 +420,13 @@ func TestLint(t *testing.T) {
 		"testdata/lint/importsnotweak/foo.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`3:1:PACKAGE_MAJOR_BETA_VERSIONED`,
+		"testdata/lint/majorbetaversioned/foo/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/majorbetaversioned/foo/foo.proto b/internal/cmd/testdata/lint/majorbetaversioned/foo/foo.proto
new file mode 100644
index 0000000..28759e5
--- /dev/null
+++ b/internal/cmd/testdata/lint/majorbetaversioned/foo/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";
+
+message Bar {
+  int64 hello = 1;
+}
diff --git a/internal/cmd/testdata/lint/majorbetaversioned/prototool.yaml b/internal/cmd/testdata/lint/majorbetaversioned/prototool.yaml
new file mode 100644
index 0000000..0fd97cf
--- /dev/null
+++ b/internal/cmd/testdata/lint/majorbetaversioned/prototool.yaml
@@ -0,0 +1,4 @@
+lint:
+  rules:
+    add:
+      - PACKAGE_MAJOR_BETA_VERSIONED
diff --git a/internal/cmd/testdata/lint/version2/prototool.yaml b/internal/cmd/testdata/lint/version2/prototool.yaml
index 3ae1b21..a095bc0 100644
--- a/internal/cmd/testdata/lint/version2/prototool.yaml
+++ b/internal/cmd/testdata/lint/version2/prototool.yaml
@@ -3,8 +3,17 @@ lint:
     add:
       - ENUM_FIELD_PREFIXES_EXCEPT_MESSAGE
       - ENUM_ZERO_VALUES_INVALID_EXCEPT_MESSAGE
+      - ENUMS_HAVE_COMMENTS
+      - FIELDS_NOT_RESERVED
       - FILE_OPTIONS_EQUAL_GO_PACKAGE_LAST_TWO_SUFFIX
-      - PACKAGE_MAJOR_VERSIONED
+      - GOGO_NOT_IMPORTED
+      - IMPORTS_NOT_PUBLIC
+      - IMPORTS_NOT_WEAK
+      - MESSAGE_FIELDS_NO_JSON_NAME
+      - MESSAGES_HAVE_COMMENTS_EXCEPT_REQUEST_RESPONSE_TYPES
+      - PACKAGE_MAJOR_BETA_VERSIONED
+      - REQUEST_RESPONSE_NAMES_MATCH_RPC
+      - RPCS_HAVE_COMMENTS
     remove:
       - ENUM_FIELD_PREFIXES
       - ENUM_ZERO_VALUES_INVALID
diff --git a/internal/cmd/testdata/reflect/one/prototool.yaml b/internal/cmd/testdata/reflect/one/prototool.yaml
index d776229..a41c2bd 100644
--- a/internal/cmd/testdata/reflect/one/prototool.yaml
+++ b/internal/cmd/testdata/reflect/one/prototool.yaml
@@ -6,7 +6,7 @@ lint:
       - FILE_OPTIONS_EQUAL_GO_PACKAGE_LAST_TWO_SUFFIX
       - GOGO_NOT_IMPORTED
       - MESSAGE_FIELDS_NO_JSON_NAME
-      - PACKAGE_MAJOR_VERSIONED
+      - PACKAGE_MAJOR_BETA_VERSIONED
       - REQUEST_RESPONSE_NAMES_MATCH_RPC
     remove:
       - ENUM_FIELD_PREFIXES
diff --git a/internal/protostrs/protostrs_test.go b/internal/protostrs/protostrs_test.go
index 8a36f63..cddf4a5 100644
--- a/internal/protostrs/protostrs_test.go
+++ b/internal/protostrs/protostrs_test.go
@@ -58,34 +58,51 @@ func TestJavaPackage(t *testing.T) {
 	assert.Equal(t, "com.foo.bar", JavaPackage("foo.bar"))
 }
 
-func TestMajorVersion(t *testing.T) {
-	version, ok := MajorVersion("foo.v0")
-	assert.True(t, ok)
-	assert.Equal(t, 0, int(version))
-	version, ok = MajorVersion("foo.v1")
-	assert.True(t, ok)
-	assert.Equal(t, 1, int(version))
-	version, ok = MajorVersion("foo.bar.v1")
-	assert.True(t, ok)
-	assert.Equal(t, 1, int(version))
-	version, ok = MajorVersion("foo.bar.v18")
-	assert.True(t, ok)
-	assert.Equal(t, 18, int(version))
-	version, ok = MajorVersion("foo.bar.v180")
-	assert.True(t, ok)
-	assert.Equal(t, 180, int(version))
-	_, ok = MajorVersion("foo.barv1")
-	assert.False(t, ok)
-	_, ok = MajorVersion("barv1")
-	assert.False(t, ok)
-	_, ok = MajorVersion("v1")
-	assert.False(t, ok)
-	_, ok = MajorVersion("foo.barv")
-	assert.False(t, ok)
-	_, ok = MajorVersion("barv")
-	assert.False(t, ok)
-	_, ok = MajorVersion("v")
-	assert.False(t, ok)
-	_, ok = MajorVersion("foo.bar.v-1")
-	assert.False(t, ok)
+func TestMajorBetaVersion(t *testing.T) {
+	testMajorBetaVersionValid(t, "foo.v1", 1, 0)
+	testMajorBetaVersionValid(t, "foo.bar.v1", 1, 0)
+	testMajorBetaVersionValid(t, "foo.bar.v18", 18, 0)
+	testMajorBetaVersionValid(t, "foo.bar.v180", 180, 0)
+	testMajorBetaVersionInvalid(t, "")
+	testMajorBetaVersionInvalid(t, "foo.v")
+	testMajorBetaVersionInvalid(t, "foo.v0")
+	testMajorBetaVersionInvalid(t, "foo.V1")
+	testMajorBetaVersionInvalid(t, "foo.barv1")
+	testMajorBetaVersionInvalid(t, "barv1")
+	testMajorBetaVersionInvalid(t, "v1")
+	testMajorBetaVersionInvalid(t, "foo.barv")
+	testMajorBetaVersionInvalid(t, "barv")
+	testMajorBetaVersionInvalid(t, "v")
+	testMajorBetaVersionInvalid(t, "foo.bar.v-1")
+	testMajorBetaVersionValid(t, "foo.v1beta1", 1, 1)
+	testMajorBetaVersionValid(t, "foo.bar.v1beta1", 1, 1)
+	testMajorBetaVersionValid(t, "foo.bar.v18beta18", 18, 18)
+	testMajorBetaVersionValid(t, "foo.bar.v180beta180", 180, 180)
+	testMajorBetaVersionInvalid(t, "foo.v0beta0")
+	testMajorBetaVersionInvalid(t, "foo.v1beta0")
+	testMajorBetaVersionInvalid(t, "foo.v0beta1")
+	testMajorBetaVersionInvalid(t, "foo.V1beta1")
+	testMajorBetaVersionInvalid(t, "foo.barv1beta1")
+	testMajorBetaVersionInvalid(t, "barv1beta1")
+	testMajorBetaVersionInvalid(t, "v1beta1")
+	testMajorBetaVersionInvalid(t, "foo.barvbeta1")
+	testMajorBetaVersionInvalid(t, "barvbeta1")
+	testMajorBetaVersionInvalid(t, "vbeta1")
+	testMajorBetaVersionInvalid(t, "foo.bar.v1beta-1")
+	testMajorBetaVersionInvalid(t, "foo.bar.v-1beta-1")
+	testMajorBetaVersionInvalid(t, "foo.v1beta")
+	testMajorBetaVersionInvalid(t, "foo.beta1")
+	testMajorBetaVersionInvalid(t, "foo.v1beta1beta1")
+}
+
+func testMajorBetaVersionValid(t *testing.T, packageName string, expectedMajorBetaVersion uint64, expectedBetaVersion uint64) {
+	majorVersion, betaVersion, ok := MajorBetaVersion(packageName)
+	assert.True(t, ok, packageName)
+	assert.Equal(t, expectedMajorBetaVersion, majorVersion, packageName)
+	assert.Equal(t, expectedBetaVersion, betaVersion, packageName)
+}
+
+func testMajorBetaVersionInvalid(t *testing.T, packageName string) {
+	_, _, ok := MajorBetaVersion(packageName)
+	assert.False(t, ok, packageName)
 }