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

fix: allow marshalling structs via interface{} #29

Merged
merged 2 commits into from
Jun 17, 2024
Merged

fix: allow marshalling structs via interface{} #29

merged 2 commits into from
Jun 17, 2024

Conversation

yan
Copy link
Contributor

@yan yan commented Jun 14, 2024

This PR allows providing structs (and struct pointers) that are defined as interface{}. e.g., this previously failed with: call of reflect.Value.NumField on interface Value

type Tree struct {
  Objs []interface{} `hcl:"objs,block"`
}

type Object struct {
  Attr string `hcl:"attr"`
}

...
t := Tree{}
t.Objs = append(x.Objs, &Object{ Attr: "hello", })
hcl.MarshalToAST(&t)

@alecthomas
Copy link
Owner

The problem is there's no way to unmarshal interfaces. At unmarshal time, there's no type information about what concrete type to unmarshal into.

@yan
Copy link
Contributor Author

yan commented Jun 15, 2024

The asymmetry is indeed not great, but being able to marshal erased values is really useful. I believe hashi's hcl supports this. What do you think about either returning error on unmarshal, or produce a map[string]interface{} which is what stdlib does, iirc?

@alecthomas
Copy link
Owner

alecthomas commented Jun 16, 2024

I think if the only interface supported is the empty one (any/interface{}), then this seems reasonable. If the type is an object it would unmarshal into a map[string]any, if it's a slice it would unmarshal into []any, a string into string, and so on.

@yan
Copy link
Contributor Author

yan commented Jun 17, 2024

@alecthomas wdyt about the above solution? i worked on a generic way to unmarshal blocks into any, but that seems like a bad idea. everything else you mentioned above should be supported.

@alecthomas
Copy link
Owner

Looks awesome, thanks Yan.

@alecthomas alecthomas merged commit 493eb71 into alecthomas:master Jun 17, 2024
2 checks passed
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.

2 participants