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

Apply defaults using custom traversal of types #574

Merged

Conversation

liamcervante
Copy link
Member

Instead of relying on the go-cty built in traversal functions we can iterate through the values when applying defaults ourselves.

This gives an advantage as we can choose when or when not to unify types. This PR introduces a small behaviour change that demonstrates the advantage. Previously, any objects or maps that had any defaults inserted would always be returned as objects to ensure there were no collisions in collections due to type differences. Now we automatically attempt to unify collection typesusing the go-cty convert package. If conversion is not possible we default to returning safe types (eg. tuples and objects) but if conversion is possible we can return the originally requested type.

ext/typeexpr/defaults.go Outdated Show resolved Hide resolved
ext/typeexpr/defaults.go Outdated Show resolved Hide resolved
Copy link
Contributor

@apparentlymart apparentlymart 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 good to me!

I re-read the test cases just to re-familiarize myself with what is valid input and expected results for this function while I was reviewing it, and I noticed a couple of test cases that don't seem to actually be valid per the rules in the function's documentation. I've proposed inline that we add an extra safety check to reject invalid use with a panic, but that's not required to fix the direct bug at hand so we could talk about that separately and consider it for a separate PR if you like.

ext/typeexpr/defaults_test.go Show resolved Hide resolved
ext/typeexpr/defaults_test.go Show resolved Hide resolved
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Since this is still largely the same as when I first reviewed it I focused mainly on the areas relevant to what we were discussing in comments above and didn't re-read the whole changeset thoroughly, but since Alisdair already reviewed it too I think that'll be okay. 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants