[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5314: Improve validation error message for regions with no division name

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #5314:
URL: https://github.com/apache/trafficcontrol/pull/5314#discussion_r527505251



##########
File path: traffic_ops/traffic_ops_golang/region/regions_test.go
##########
@@ -109,3 +109,30 @@ func TestInterfaces(t *testing.T) {
 		t.Errorf("Region must be Identifier")
 	}
 }
+func TestValidation(t *testing.T) {
+	testRegion := tc.Region{
+		DivisionName: "west",
+		ID:           1,
+		Name:         "region1",
+		LastUpdated:  tc.TimeNoMod{Time: time.Now()},
+	}
+	testTORegion := TORegion{Region: testRegion}
+	errs := test.SortErrors(test.SplitErrors(testTORegion.Validate()))
+	expected := []error{}
+
+	if reflect.DeepEqual(expected, errs) {

Review comment:
       You should avoid using `reflect.DeepEqual` to compare error values in tests - and actually I think `golang-lint` warns about this by default. The reason is that errors are consistently appended to outer errors to build an idea of the scope of the error and at what level it occurred. Because of that or just adding more context, error strings change all the time, so using `reflect.DeepEqual` makes tests brittle.
   
   Plus in this particular case, you're only looking for an empty set of errors, so you can easily check `len(errs) != 0` instead.

##########
File path: traffic_ops/traffic_ops_golang/region/regions.go
##########
@@ -95,7 +95,11 @@ func (region *TORegion) GetType() string {
 
 func (region *TORegion) Validate() error {
 	if len(region.Name) < 1 {
-		return errors.New(`Region 'name' is required.`)
+		return errors.New(`region 'name' is required`)
+	}
+	if region.DivisionName == "" || region.Division == 0 {
+		// golang style guides say error strings shouldn't be capitalized or end with punctuation

Review comment:
       NIT: This is true, but you don't need to mention it. Otherwise it could go above every instantiation of an error, and that's how we wound up with
   
   ```go
   //this utilizes the non panicking type assertion, if the thrown away ok variable is false i will be the zero of the type, 0 here.
   ```
   pasted a million times all over the codebase.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org