consolidate default value type comparisons into local bucketing code #247
Conversation
ajwootto
commented
Apr 22, 2024
•
ajwootto
commented
- move the value type comparison happening in client.go into the bucketing code and report on it with a variable defaulted (before if this case was hit it would be tracked as a variable evaluated)
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| @@ -212,7 +214,9 @@ func GenerateBucketedConfig(sdkKey string, user api.PopulatedUser, clientCustomD | |||
| }, nil | |||
| } | |||
|
|
|||
| func VariableForUser(sdkKey string, user api.PopulatedUser, variableKey string, expectedVariableType string, eventQueue *EventQueue, clientCustomData map[string]interface{}) (variableType string, variableValue any, err error) { | |||
| func VariableForUser(sdkKey string, user api.PopulatedUser, variableKey string, defaultValue interface{}, eventQueue *EventQueue, clientCustomData map[string]interface{}) (variableType string, variableValue any, err error) { | |||
| expectedVariableType, err := variable_utils.VariableTypeFromValue(variableKey, defaultValue, true) | |||
| @@ -222,9 +226,17 @@ func VariableForUser(sdkKey string, user api.PopulatedUser, variableKey string, | |||
| return "", nil, err | |||
| } | |||
|
|
|||
| if !isVariableTypeValid(variableType, expectedVariableType) && expectedVariableType != "" { | |||
| typeFieldMismatch := !isVariableTypeValid(variableType, expectedVariableType) && expectedVariableType != "" | |||
There was a problem hiding this comment.
This kind of one liner is discouraged - and should be done in the if statement to make it more transparent.
There was a problem hiding this comment.
i found it so much harder to read the if statement condition when i inlined it all
There was a problem hiding this comment.
if you really want to make it oneliner'd - move the explicit variable call checks first to make it more visible.
| case variable_utils.ErrInvalidDefaultValue: | ||
| return "INVALID_DEFAULT_VALUE" |
There was a problem hiding this comment.
We should make these accessible constants still.
| "fmt" | ||
| "reflect" | ||
| ) | ||
|
|
There was a problem hiding this comment.
You could add the default reasons here
There was a problem hiding this comment.
feels like they should go in the same place as the error definitions that cause them, which is all in the bucketing package
There was a problem hiding this comment.
really it doesn't matter where - they can be imported from anywhere
