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

fluent: provide an API to convert an ipld.Node to a generic Go value #178

Closed
mvdan opened this issue May 25, 2021 · 6 comments · Fixed by #304
Closed

fluent: provide an API to convert an ipld.Node to a generic Go value #178

mvdan opened this issue May 25, 2021 · 6 comments · Fixed by #304
Assignees
Labels
good first issue Good issue for new contributors

Comments

@mvdan
Copy link
Contributor

mvdan commented May 25, 2021

In other words, the reverse of fluent.Reflect. Int gives an int64, Map gives a map[string]interface{}, List gives a []interface{}, and so on.

This is somewhat similar to how code-generated scalar nodes have methods like Int() int64 as well as the usual AsInt() (int64, error), but applied more generally for any node implementation and kind.

The main use case is when one has small IPLD nodes, such as any scalar kind or a small list/map, and one wants to obtain the plain Go value for easy use. Especially for debugging or testing, too.

This could potentially allocate a lot for large maps or lists, but we assume noone would do that.

Unfortunately this does mean that calling this API on a List_String would return a []interface{} instead of []string, which is unfortunate and not terribly helpful. But I don't think we can reasonably do better right now. Perhaps we can revisit that with generics.

cc @warpfork @willscott for the recent discussion
cc @gammazero since your recent question made me reconsider proposing this

@mvdan
Copy link
Contributor Author

mvdan commented May 25, 2021

Also, I'm intentionally avoiding the name bikeshedding until we agree that we want this.

@warpfork
Copy link
Collaborator

warpfork commented Jun 9, 2021

Agree useful.

Agree it would allocate a lot for large structures, and agree we should also just document that and say use it at your own (performance) risk.

Agree that it would return []interface{} rather than []string, and probably do that even if we had a schema somewhere which had more info about it. But this is also very comparable to what stdlib json will do, so it's not going to be at all surprising or unusual in golang, I think.

@warpfork
Copy link
Collaborator

warpfork commented Jun 9, 2021

This would also let us bang out some interesting quick tests and examples like: take a dag-json fixture; parse to ipld.Node; apply fluent.Dereflect (... naming...) to get interface{}; apply stdlib json marshal; expect to get something equal to the dag-json fixture (modulo any CIDs or bytes).

... not that anyone would ever really need to do that, but the example could be helpful for showing people what these concepts can be compared to.

@rvagg
Copy link
Member

rvagg commented Jun 9, 2021

What about going the same route as json et. al. with interface{} by being able to marshal and unmarshal into structs annotated with tags? It'd then offer an idiomatic and comfortable way for people to interface with ipld-prime if performance wasn't critical or they needed an easy interface to existing systems?

@warpfork
Copy link
Collaborator

warpfork commented Jun 9, 2021

That's what @mvdan's also doing in the new node/bindnode package :) It's just still going to be somewhat higher-touch to use it, generally.

It's also just much less code involved to do an unpack into interface{}.

(Standard library golang json even has a special path when it notices it's unpacking into interface{}, because it also gets to take even more shortcuts then. So that's definitely an interesting note on prior art finding it useful to have such a codepath as this.)

@mvdan mvdan added the good first issue Good issue for new contributors label Nov 12, 2021
@MarcoPolo
Copy link
Contributor

I'll take this @mvdan, can you assign this to me please?

@mvdan mvdan closed this as completed in #304 Dec 2, 2021
mvdan pushed a commit that referenced this issue Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants