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

Template function #37

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Template function #37

merged 2 commits into from
Dec 20, 2023

Conversation

lukaso
Copy link
Contributor

@lukaso lukaso commented Dec 20, 2023

@lukaso lukaso marked this pull request as ready for review December 20, 2023 17:59
@lukaso
Copy link
Contributor Author

lukaso commented Dec 20, 2023


switch {

case val.Type().Equals(cty.String):
Copy link
Contributor

Choose a reason for hiding this comment

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

val.Type() can be done once at the top of the switch statement and then have case: cty.String etc instead

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
Contributor Author

Choose a reason for hiding this comment

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

I think some of the types can't be done that way. I can get type once however.

Here's their code from go-cty/cty](https://github.com/zclconf/go-cty/tree/v1.13.2/cty) /type.go:

func (t Type) WithoutOptionalAttributesDeep() Type {
	switch {
	case t == DynamicPseudoType, t.IsPrimitiveType(), t.IsCapsuleType():
		return t
	case t.IsMapType():
		return Map(t.ElementType().WithoutOptionalAttributesDeep())
	case t.IsListType():
		return List(t.ElementType().WithoutOptionalAttributesDeep())
	case t.IsSetType():
		return Set(t.ElementType().WithoutOptionalAttributesDeep())
	case t.IsTupleType():
		originalElemTypes := t.TupleElementTypes()
		elemTypes := make([]Type, len(originalElemTypes))
		for i, et := range originalElemTypes {
			elemTypes[i] = et.WithoutOptionalAttributesDeep()
		}
		return Tuple(elemTypes)
	case t.IsObjectType():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed to get type only once.

dsl/template.go Outdated
Comment on lines 40 to 41
// Check if variablesVal is a string and try to json.Unmarshal it
if variablesVal.Type() == cty.String {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on this, it's an extra complication that adds a gotcha.

I'd just take the argument as a typed object and drop this implicit string to json conversion. They can already convert it to an object via jsondecode if that's what they meant, but the logic above means they have no choice in the matter. Would be frustrating for users if they ever actually want to pass a json string through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no problem. Makes sense. I would say, they can't pass just a json string, because the template can't handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

dsl/template.go Outdated
Comment on lines 75 to 77
if filename == "" {
return "", nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Task said this must return an error for an unknown template file, this silently fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Also added file not found error in file() function.

@lukaso lukaso merged commit 42162a6 into main Dec 20, 2023
@lukaso lukaso deleted the template-function branch December 20, 2023 20:11
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.

None yet

2 participants