FF-1939 Update to UFC #41
Conversation
Instead of copying just selected files/directories, have the full repository available to tests.
|
|
||
| // Find matching rule | ||
| rule, err := findMatchingRule(subjectAttributes, config.Rules) | ||
| err = flag.verifyType(variationType) |
There was a problem hiding this comment.
In swift (and android and python) we had to also handle when the variation type did not match the desired kind. specifically the invalid-value-flag test where an INTEGER variation type is desired but a double is received. does that get handled for you in a way i'm not seeing? maybe valueToAssignmentValue is handling that
There was a problem hiding this comment.
Yes. That's handled in valueToAssignmentValue: https://github.com/Eppo-exp/golang-sdk/pull/41/files/80ac9ea3cf91ce0e12a64b97526135d674f4b16d#diff-05660c7f68f1998db7bb1977133831b136836ad85000bedcf3bd0917aee448f6R89
We can't handle this verification here yet as it depends on which variant is selected
| return defaultValue, err | ||
| } | ||
| result, _ := variation.(bool) | ||
| return result, err |
There was a problem hiding this comment.
I (vaguely) know that this is a common Go pattern, but given that we return the default value when an error occurs, do we need to return the error here? Is the user ever going to care about what err is?
There was a problem hiding this comment.
Mostly for debugging. The error might be helpful to understand why default value is returned
There was a problem hiding this comment.
Yes in theory we can:
return result, nil
That would show "us" that everything is fine at this point. In practice err should be nil since we handled both causes of non-nil on line 29 and 33
| if !ok { | ||
| return expConfig, errors.New("configuration not found in configuration store") | ||
| return flag, errors.New("configuration not found in configuration store") |
There was a problem hiding this comment.
No, it's "empty"—all fields are set to default values. (Can't return nil from here as the return type is flagConfiguration not *flagConfiguration.)
|
Test case is failing because |
| return defaultValue, err | ||
| } | ||
| result, _ := variation.(bool) | ||
| return result, err |
There was a problem hiding this comment.
Yes in theory we can:
return result, nil
That would show "us" that everything is fine at this point. In practice err should be nil since we handled both causes of non-nil on line 29 and 33
No description provided.