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/basetypes: Add Tuple type support #870

Merged
merged 18 commits into from
Nov 3, 2023
Merged

types/basetypes: Add Tuple type support #870

merged 18 commits into from
Nov 3, 2023

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented Nov 2, 2023

Ref: #54

This PR introduces basetypes.TupleType, basetypes.TupleValue, and tuple helper functions/aliases in the types package. This PR does not introduce any accompanying TupleAttribute implementations, so currently there is no way to describe this schema to Terraform core.

Eventually when #147 is implemented, the TupleType will be necessary to fully handle dynamic attributes, as then Terraform core could possible send a tuple to Plugin Framework.

Notes

  • I updated the reflection rules to create an explicit message describing that reflection from Go slices into types.Tuple is not supported. Also added tests around the code that was refactored relating to sets/lists
    • There weren't explicit tests for Into and FromValue, so I created table tests for them and added tests for just what I refactored. We can continually update the tests as we make changes to internal/reflect
  • I wasn't 100% sure if we needed to implement (TupleType).ApplyTerraform5AttributePathStep for dynamic attributes, so I can update the implementation to return an error if that's not needed.

@austinvalle austinvalle requested a review from a team as a code owner November 2, 2023 20:00
@austinvalle austinvalle added this to the v1.5.0 milestone Nov 2, 2023
@austinvalle
Copy link
Member Author

For some reason I forgot that the documentation existed in this repo 😖, updated the PR with the tuple docs ⚡

Copy link
Contributor

@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.

This looks great to me 🚀 Just a few minor suggestions.

I wasn't 100% sure if we needed to implement (TupleType).ApplyTerraform5AttributePathStep for dynamic attributes, so I can update the implementation to return an error if that's not needed.

Adding the correct implementation now (which it looks like you did), rather than potentially discovering that as an issue later, is definitely appreciated!

.changes/unreleased/FEATURES-20231102-160132.yaml Outdated Show resolved Hide resolved
internal/reflect/into_test.go Outdated Show resolved Hide resolved
internal/reflect/outof_test.go Outdated Show resolved Hide resolved
types/basetypes/tuple_value.go Outdated Show resolved Hide resolved
@@ -83,7 +83,7 @@ Call one of the following to create a `types.List` value:
* [`types.ListUnknown(attr.Type) types.List`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types#ListUnknown): An unknown list value with the given element type.
* [`types.ListValue(attr.Type, []attr.Value) (types.List, diag.Diagnostics)`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types#ListValue): A known value with the given element type and values.
* [`types.ListValueFrom(context.Context, attr.Type, any) (types.List, diag.Diagnostics)`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types#ListValueFrom): A known value with the given element type and values. This can convert the source data from standard Go types into framework types as noted in the documentation for each element type, such as giving `[]*string` for a `types.List` of `types.String`.
* [`types.ListValueMust(map[string]attr.Type, map[string]attr.Value) types.List`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types#ListValueMust): A known value with the given element type and values. Any diagnostics are converted to a runtime panic. This is recommended only for testing or exhaustively tested logic.
* [`types.ListValueMust(attr.Type, []attr.Value) types.List`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types#ListValueMust): A known value with the given element type and values. Any diagnostics are converted to a runtime panic. This is recommended only for testing or exhaustively tested logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely did NOT find this because I was copy pasting the docs 😅

@bflad bflad added enhancement New feature or request types Issues and pull requests about our types abstraction and implementations. labels Nov 3, 2023
@austinvalle austinvalle merged commit ce031ec into main Nov 3, 2023
20 checks passed
@austinvalle austinvalle deleted the av/tuples branch November 3, 2023 16:41
Copy link

github-actions bot commented Dec 4, 2023

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 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants