Clarify some error messages and doc comments.

sdg0919 · Jun 15, 2016 · 5e4f638 · 5e4f638
1 parent 2613829
commit 5e4f638
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 12 deletions.
diff --git a/logfwd/origin.go b/logfwd/origin.go
@@ -14,7 +14,6 @@ const canonicalIANAid = "28978"
 
 // These are the recognized origin types.
 const (
-	originTypeInvalid OriginType = -1
 	OriginTypeUnknown OriginType = 0
 	OriginTypeUser               = iota
 )
@@ -24,7 +23,18 @@ var originTypes = map[OriginType]string{
 	OriginTypeUser:    names.UserTagKind,
 }
 
-// OriginType is the "enum" type for the different kinds of log record origin.
+// OriginType is the "enum" type for the different kinds of log record
+// origin.
+//
+// Go does not have native enum support, nor support for struct literal
+// constants.  Thus to use constants for the enum values we must
+// "typedef" a type that Go supports for constants (e.g. int). One
+// downside is that the underlying concrete value is exposed to type
+// conversion as well as the operators for the underlying type. It also
+// means that any value of the underlying type may be type converted to
+// the enum type, even though the enum type doesn't support the value.
+// Consequently the enum type must have a Validate() method to ensure
+// this did not happen.
 type OriginType int
 
 // ParseOriginType converts a string to an OriginType or fails if
@@ -35,6 +45,7 @@ func ParseOriginType(value string) (OriginType, error) {
 			return ot, nil
 		}
 	}
+	const originTypeInvalid OriginType = -1
 	return originTypeInvalid, errors.Errorf("unrecognized origin type %q", value)
 }
 
@@ -45,11 +56,11 @@ func (ot OriginType) String() string {
 
 // Validate ensures that the origin type is correct.
 func (ot OriginType) Validate() error {
-	// Ideally, only the (unavoidable) zero value would be invalid.
-	// However, typedef'ing int means that the use of int literals
-	// could result in invalid values other than the zero value.
+	// As noted above, typedef'ing int means that the use of int
+	// literals or explicit type conversion could result in unsupported
+	// "enum" values. Otherwise OriginType would not need this method.
 	if _, ok := originTypes[ot]; !ok {
-		return errors.NewNotValid(nil, "unknown origin type")
+		return errors.NewNotValid(nil, "unsupported origin type")
 	}
 	return nil
 }
@@ -64,7 +75,7 @@ func (ot OriginType) ValidateName(name string) error {
 		}
 	case OriginTypeUser:
 		if !names.IsValidUser(name) {
-			return errors.NewNotValid(nil, "bad user")
+			return errors.NewNotValid(nil, "bad user name")
 		}
 	}
 	return nil

diff --git a/logfwd/origin_test.go b/logfwd/origin_test.go
@@ -94,7 +94,7 @@ func (s *OriginTypeSuite) TestValidateInvalid(c *gc.C) {
 	err := ot.Validate()
 
 	c.Check(err, jc.Satisfies, errors.IsNotValid)
-	c.Check(err, gc.ErrorMatches, `unknown origin type`)
+	c.Check(err, gc.ErrorMatches, `unsupported origin type`)
 }
 
 func (s *OriginTypeSuite) TestValidateNameValid(c *gc.C) {
@@ -123,7 +123,7 @@ func (s *OriginTypeSuite) TestValidateNameInvalid(c *gc.C) {
 	}, {
 		ot:   logfwd.OriginTypeUser,
 		name: "...",
-		err:  `bad user`,
+		err:  `bad user name`,
 	}}
 	for _, test := range tests {
 		c.Logf("trying %q + %q", test.ot, test.name)
@@ -220,7 +220,7 @@ func (s *OriginSuite) TestValidateBadOriginType(c *gc.C) {
 	err := origin.Validate()
 
 	c.Check(err, jc.Satisfies, errors.IsNotValid)
-	c.Check(err, gc.ErrorMatches, `invalid Type: unknown origin type`)
+	c.Check(err, gc.ErrorMatches, `invalid Type: unsupported origin type`)
 }
 
 func (s *OriginSuite) TestValidateEmptyName(c *gc.C) {
@@ -240,7 +240,7 @@ func (s *OriginSuite) TestValidateBadName(c *gc.C) {
 	err := origin.Validate()
 
 	c.Check(err, jc.Satisfies, errors.IsNotValid)
-	c.Check(err, gc.ErrorMatches, `invalid Name "...": bad user`)
+	c.Check(err, gc.ErrorMatches, `invalid Name "...": bad user name`)
 }
 
 func (s *OriginSuite) TestValidateEmptyVersion(c *gc.C) {

diff --git a/logfwd/record_test.go b/logfwd/record_test.go
@@ -44,7 +44,7 @@ func (s *RecordSuite) TestValidateBadOrigin(c *gc.C) {
 	err := rec.Validate()
 
 	c.Check(err, jc.Satisfies, errors.IsNotValid)
-	c.Check(err, gc.ErrorMatches, `invalid Origin: invalid Name "...": bad user`)
+	c.Check(err, gc.ErrorMatches, `invalid Origin: invalid Name "...": bad user name`)
 }
 
 func (s *RecordSuite) TestValidateEmptyTimestamp(c *gc.C) {