-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
types: Support Float64, Float64Type, Int64, and Int64Type #166
Conversation
types/float64.go
Outdated
} | ||
|
||
// Validation is handled by Validate method. | ||
f, _ := bigF.Float64() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say let's still bubble up the error anyways, for when things don't work the way they're supposed to. Error checking in depth. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm not positive of this, but an interesting caveat to this is that this error I think will always be returned first instead of the Validate()
diagnostics. I guess the only real difference is the error messaging/formatting, but could be something to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it? I thought we designed Validate
diagnostics to run before trying to create any attr.Value
s, which is (as far as I can tell) where this gets used? Otherwise, the Validate
would take an attr.Value
argument, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terraform-plugin-framework/tfsdk/config.go
Lines 46 to 73 in 84afb9a
tfValue, err := c.terraformValueAtPath(path) | |
if err != nil { | |
diags.AddAttributeError( | |
path, | |
"Configuration Read Error", | |
"An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:\n\n"+err.Error(), | |
) | |
return nil, diags | |
} | |
if attrTypeWithValidate, ok := attrType.(attr.TypeWithValidate); ok { | |
diags.Append(attrTypeWithValidate.Validate(ctx, tfValue, path)...) | |
if diags.HasError() { | |
return nil, diags | |
} | |
} | |
attrValue, err := attrType.ValueFromTerraform(ctx, tfValue) | |
if err != nil { | |
diags.AddAttributeError( | |
path, | |
"Configuration Read Error", | |
"An unexpected error was encountered trying to read an attribute from the configuration. This is always an error in the provider. Please report the following to the provider developer:\n\n"+err.Error(), | |
) | |
return nil, diags | |
} |
Validate
diags will be returned instead of the ValueFromTerraform
error when both are returned. I think the reason I'm arguing we should handle it here anyways is as a safeguard in case we ever forget to call Validate
or call them in the wrong order or something.
…rs, refactor validate functions into type files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 🚀
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. |
Closes #137