Skip to content
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: Add Typable and Valuable Interfaces #536

Merged
merged 12 commits into from Nov 15, 2022
Merged

Conversation

bendbennett
Copy link
Contributor

Closes: #535

… types.ListType and types.List (value), respectively (#535)
… types.MapType and types.Map (value), respectively (#535)
… types.SetType and types.Set (value), respectively (#535)
… types.ObjectType and types.Object (value), respectively (#535)
… primitive types and primitive values, respectively (#535)
@bendbennett bendbennett marked this pull request as ready for review November 11, 2022 15:29
@bendbennett bendbennett requested a review from a team as a code owner November 11, 2022 15:29
@bflad bflad self-assigned this Nov 11, 2022
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks like a really good direction. We'll also want to ensure there's Go documentation for the new interfaces and methods.

types/list.go Outdated
)

type ListTyp interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To potentially make the interface naming clearer, we may want to consider using -er (🙁 ) or -able (😄 ) naming. e.g. ListTypable and ListValuable 💰

types/list.go Outdated
type ListTyp interface {
attr.Type

ValueFromFramework(context.Context, List) (attr.Value, diag.Diagnostics)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we can do since we are creating separate interfaces is name the methods to match the types now 🎉 . I'm not sure that we necessarily have a use case for it (maybe this could help if we ever wanted to implement dynamic types?), but it would then also allow custom types to support multiple type implementations.

Another thing we should likely consider here is that the returned attr.Value can be constrained further to only matching the value interface specific to the type. e.g. in this case ListVal / ListValuable

ValueFromList(context.Context, List) (ListValuable, diag.Diagnostics)

types/list.go Outdated
type ListVal interface {
attr.Value

ToFrameworkValue(ctx context.Context) (List, diag.Diagnostics)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly with these

ToListValue(ctx context.Context) (List, diag.Diagnostics)

types/bool.go Show resolved Hide resolved
types/bool.go Show resolved Hide resolved
types/list.go Outdated
type ListTypable interface {
attr.Type

ValueFromList(context.Context, List) (attr.Value, diag.Diagnostics)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing we should likely consider for these is that the returned attr.Value can be constrained further to only allowing the value interface specific to the type.

// ValueFromList should return the equivalent value from a given List.
ValueFromList(context.Context, List) (ListValuable, diag.Diagnostics)

Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're getting close here!

types/list.go Outdated
Comment on lines 22 to 23
// ListTypable extends attr.Type for list type types.
// Implement this interface to create a custom ListType type type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we can just refer to the "type type" as just "type" and leave the distinction of "value type" to be separate 😎

Suggested change
// ListTypable extends attr.Type for list type types.
// Implement this interface to create a custom ListType type type.
// ListTypable extends attr.Type for list types.
// Implement this interface to create a custom ListType type.

)

// ListTypable extends attr.Type for list type types.
// Implement this interface to create a custom ListType type type.
type ListTypable interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add changelog entries for these? e.g.

types: Added `ListTypable` and `ListValuable` interface types, which enable embedding existing list types for custom types

Or something along those lines?

"Attribute Validation Error",
"Attribute validation cannot walk schema. Report this to the provider developer:\n\n"+err.Error(),
"Attribute Validation Error Invalid Value Type",
"A type from which a types.List can be obtained is expected here. Report this to the provider developer:\n\n"+err.Error(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should we give the direct hint about the types.ListValuable interface for these?

)

return
}

l, diags := listVal.ToListValue(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're ready for unit testing these! Maybe some additional types in internal/testing/types?

@bflad bflad added this to the v0.16.0 milestone Nov 14, 2022
@bflad bflad added the enhancement New feature or request label Nov 14, 2022
… attribute plan modification and attribute validation for custom nested attribute types (#535)
@bflad bflad changed the title attr: Add ValueFromFramework and ToFrameworkValue Interface Functions to attr.Value types: Add Typable and Valuable Interfaces Nov 14, 2022
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀 Nice work.

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attr: Add ValueFromFramework and ToFrameworkValue Interface Functions to attr.Value
2 participants