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

Simplify Extending Primitive Types #91

Closed
bflad opened this issue Aug 10, 2021 · 5 comments · Fixed by #567
Closed

Simplify Extending Primitive Types #91

bflad opened this issue Aug 10, 2021 · 5 comments · Fixed by #567
Labels
breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. enhancement New feature or request types Issues and pull requests about our types abstraction and implementations.
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Aug 10, 2021

Module version

v0.2.0

Use-cases

Provider developers may wish to implement higher level types based off the primitive types in the current types package. For example, this may be a string-based attribute type that implements common validation checks on top.

Attempted Solutions

Currently, this seems more difficult than it should be since these higher level types cannot extend BoolType, NumberType, and StringType. A current possible workaround can be seen in the internal/testing/types package, which reimplements those types as structures.

Trying with type aliasing fails:

type BoolTypeWithValidateError types.BoolType
types.BoolType (constant 2 of type types.primitive) is not a type (NotAType)

Creating a struct type (with pointer or concrete) fails:

type BoolTypeWithValidateError struct {
	types.BoolType
}
types.BoolType (constant 2 of type types.primitive) is not a type (NotAType)

Trying as a variable also sensibly fails:

var BoolTypeWithValidateError = types.BoolType

func (t BoolTypeWithValidateError) Validate(ctx context.Context, in tftypes.Value) []*tfprotov6.Diagnostic {
	return []*tfprotov6.Diagnostic{TestErrorDiagnostic}
}
BoolTypeWithValidateError (variable of type types.primitive) is not a type (NotAType)

Other current types, such as ListType, MapType, and ObjectType are not affected as they are already structure types.

Proposal

This functionality should be designed with a holistic view on custom types, as such it has been marked with the design-doc label to signify that extra documentation should be provided on decision making.

One possible path forward would be to switch BoolType, NumberType, and StringType to structure types, similar to what is done in the current internal/testing/types package.

References

@bflad bflad added enhancement New feature or request design-doc Issues or pull requests about a design document, capturing design ideas and trade-offs. labels Aug 10, 2021
@paddycarver
Copy link
Contributor

Agree with the thrust of this, just dropping in the additional context that they started out as struct types, and we moved to the current implementation to avoid people needing to type {} after them everywhere.

@paddycarver paddycarver added the types Issues and pull requests about our types abstraction and implementations. label Sep 21, 2021
@bflad bflad added this to the v1.0.0 milestone Mar 16, 2022
@tmccombs
Copy link

tmccombs commented Apr 9, 2022

and we moved to the current implementation to avoid people needing to type {} after them everywhere.

Maybe have a Const that is defined as StringType{}?

@paddycarver
Copy link
Contributor

I'm no longer working on this, but if memory serves the underlying problem here is that primitives are of an unexported primitive type and share an implementation, with each primitive getting its own constant.

In theory, primitives could be changed to their own types instead of being the unexported primitive type, but that would require them to be instantiated (including {} if they're a struct type, including ("") if they're a string type, including (0) if they're an integer type, etc.)

Primitives could be changed to their own types instead of being the unexported primitive type and a constant for each could be created, but what would the type/constant be named? Either the constant is named unintuitively for "normal" use or the type is named unintuitively for use as a base type implementation.

The unexported primitive type could be exported, but that exposes a lot more surface area that wasn't intended to be exposed, and I'm not sure that design makes sense.

I think my pitch would be to have a github.com/hashicorp/terraform-plugin-framework/types/base package or something that implemented types that were built to be extended, and maybe even refactor github.com/hashicorp/terraform-plugin-framework/types to extend those base types. Then "normal" usage would still be types.StringType but you could extend from base.StringType if you wanted a special kind of string.

@bflad bflad added breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. and removed design-doc Issues or pull requests about a design document, capturing design ideas and trade-offs. labels Nov 9, 2022
bflad added a commit that referenced this issue Nov 30, 2022
Reference: #91

Aliasing and function shadowing in the original `types` package should prevent most provider developer changes. The main exception is the newer type-specific `Typable` and `Valuable` interfaces were moved without a type alias. This should help developers find the necessary interfaces for custom types next to the base type implementations.

The underlying implementation of the primitive types (`BoolType`, `Float64Type`, `Int64Type`, `NumberType`, and `StringType`) are now fully exported types instead of the unexported `primitive` type which was difficult to extend.

The underlying value type creation functions were prefixed with New and the value types themselves were renamed to include Value at the end. This should prevent rough edges with the `String` value type since it conflicted with the `String()` method and could not be directly embedded easily.
bflad added a commit that referenced this issue Dec 12, 2022
Reference: #91

Aliasing and function shadowing in the original `types` package should prevent most provider developer changes. The main exception is the newer type-specific `Typable` and `Valuable` interfaces were moved without a type alias. This should help developers find the necessary interfaces for custom types next to the base type implementations.

The underlying implementation of the primitive types (`BoolType`, `Float64Type`, `Int64Type`, `NumberType`, and `StringType`) are now fully exported types instead of the unexported `primitive` type which was difficult to extend.

The underlying value type creation functions were prefixed with New and the value types themselves were renamed to include Value at the end. This should prevent rough edges with the `String` value type since it conflicted with the `String()` method and could not be directly embedded easily.
@bflad
Copy link
Contributor Author

bflad commented Dec 12, 2022

Similar to the above comment, a types/basetypes package has been created which contains the extendable Go types for implementing custom types. Aliasing has been setup so existing types usage in providers should not require changes.

Maybe for the next major version, we can consider cleaning this up slightly, but this should make life easier for now.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. enhancement New feature or request types Issues and pull requests about our types abstraction and implementations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants