Adding Map validation for KeysAre and ValuesAre#38
Conversation
| for k := range elems { | ||
| request := tfsdk.ValidateAttributeRequest{ | ||
| AttributePath: req.AttributePath.WithElementKeyString(k), | ||
| AttributeConfig: types.String{Value: k}, |
There was a problem hiding this comment.
This seems a bit "hacky" to me. If a validator that is subsequently called by ranging over v.keyValidators uses the AttributePath to obtain the value for performing the validation this will break. Interested to hear opinions.
There was a problem hiding this comment.
I think this is something to call in the Go documentation for the validator/function, but expected in this usage. The intention of the validator is to loop through the string map keys to perform string validations against those keys and the ValuesAre validator is separate for this reason.
There was a problem hiding this comment.
I've updated the docs to provide some clarification around this usage.
|
Following discussion with @detro we could consolidate the |
| keysAreValidators: []tfsdk.AttributeValidator{ | ||
| stringvalidator.LengthAtLeast(4), | ||
| }, | ||
| expectError: true, |
There was a problem hiding this comment.
Nit: At some point it might be nice to refactor these validator tests to test the expected error messages, rather than just being conditional that some error was raised.
| gotMapElems, gotOk := validateMap(context.Background(), testCase.request, &tfsdk.ValidateAttributeResponse{}) | ||
|
|
||
| if diff := cmp.Diff(gotMapElems, testCase.expectedMap); diff != "" { | ||
| t.Errorf("unexpected float64 difference: %s", diff) |
|
|
||
| for k := range elems { | ||
| request := tfsdk.ValidateAttributeRequest{ | ||
| AttributePath: req.AttributePath.WithElementKeyString(k), |
There was a problem hiding this comment.
Excellent choice here to highlight the whole key/value (at least I think it'll highlight the key too, but regardless, we do not have a way to signal upstream just a map key so this is a good compromise rather than just highlighting the whole map).
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
2 participants
Closes: #13