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

Introduce fluent.Reflect convenience functions. #81

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Conversation

warpfork
Copy link
Collaborator

This diff introduces new convenience functions into the fluent package which create Nodes by looking at data in golang native structures and flipping them into IPLD data model.

These are probably not fast, but they might be handy in debugging, putting together quick hacks, or simply for demos.

@warpfork
Copy link
Collaborator Author

warpfork commented Sep 18, 2020

I haven't filled out the reflect switch completely for all scalar types yet, but that's a SMOP. The recursive bits appear to all be in place and working correctly.

The bigger question is: do we actually want to carry something like this? Does it belong in this package? Etc.

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

I think this is useful, and we should use it for quick demos. We would end up with three ways for Go devs to build nodes:

  1. fluent's Reflect, which is easy and intuitive, but pretty expensive.
  2. fluent's Build*, which is a bit more verbose but more powerful.
  3. ignoring fluent and using ipld straight away. the most low level, so the most verbose but also the most efficient.

fluent/reflect.go Show resolved Hide resolved
fluent/reflect_test.go Outdated Show resolved Hide resolved
@mvdan
Copy link
Contributor

mvdan commented Sep 18, 2020

Ideally we'd eventually keep only 2 ways to build nodes, one high-level and one low-level. But for now, I think three is reasonable given that the first two are in the fluent package.

@warpfork
Copy link
Collaborator Author

There has also been the idea of having a whole "bind.Node" package which could handle more complex and configurable relationships to go native types using reflection. (Perhaps using something like refmt's atlases for configuration, or perhaps using tagging conventions.)

That's probably a different beast, though, simply because it's a significantly bigger endeavor than this.

@mvdan
Copy link
Contributor

mvdan commented Sep 23, 2020

I'll leave it to you to merge, by the way.

@warpfork
Copy link
Collaborator Author

I still need to give this another pass and make sure all the switches covers all the cases for reflect.Kind -- haven't filled in all the variations on int sizes, etc, yet. Once that's done, it'll be ready to merge.

@warpfork
Copy link
Collaborator Author

I've added more cases for all the other primitives that are reasonable to convert, which makes this about ready to merge.

@mvdan, are you familiar with the reflection rules about exported fields? I was going to make a test for pointers, but got an odd error from it; something like ->

	t.Run("Pointer", func(t *testing.T) {
		type Foo string
		type Bar struct {
			Z *Foo
		}
		// ...

... caused me to receive the panic "reflect.Value.Interface: cannot return value obtained from unexported field or method", which... doesn't really make any sense to me. The same setup without the * works just fine.

@warpfork
Copy link
Collaborator Author

Also raises the question of what we should do on unexported fields. I think the panic may actually be fine, given that this is supposed to be a rough and scrappy rapid-prototyping/kludge function. Thoughts?

(Needs documentation, either way.)

@mvdan
Copy link
Contributor

mvdan commented Sep 28, 2020

I have no idea how you got that pointer error, sorry. If you can provide a reproducer, I can dig into it later today.

I agree that panicking on a bad type is fine. Those should be static, so it should either always or never panic. Plus, like you say, this is a shortcut func.

@warpfork
Copy link
Collaborator Author

warpfork commented Oct 1, 2020

The full hunk of tests that gives me the error is:

	t.Run("Pointer", func(t *testing.T) {
		type Zoo string
		type Zar struct {
			Z *Zoo
		}
		foo := Zoo("foo")
		n, err := fluent.Reflect(basicnode.Prototype.Any, Zar{&foo})
		Wish(t, err, ShouldEqual, nil)
		Wish(t, n.ReprKind(), ShouldEqual, ipld.ReprKind_Map)
		Wish(t, must.String(must.Node(n.LookupByString("Z"))), ShouldEqual, "foo")
	})

and it's panicking from the line that's launching recursion into struct fields and attempting to unwrap with a call of fv.Interface().

The first couple things I thought might be sources of oddness were the use of a "_test" package declaration, and the declaration of types that aren't package scoped, but I tried removing both of those from play and still got the panic. At this point I'm just confused because I'm pretty sure "Z" is a capital letter (but I'm sure I'll headdesk as soon as we spot the issue...).

(Also, yeah, this could be rewritten to be more efficient: flipping the reflect.Value back to an interface{} for every recursion is good for code brevity, horrible for performance. Doing the rewrite for that might also dodge this panic.)

@warpfork
Copy link
Collaborator Author

warpfork commented Oct 1, 2020

I've also just added a bunch of docs to these methods; PTAL at that also real quick :D

(I'd like to add some usage examples, too, but... I've filed #88 for what's stopping me there, and I think I am going to attack that yak-shave directly because it's come up somewhat recurringly. So examples for this feature probably won't come in this PR, but a subsequent one instead.)

@warpfork
Copy link
Collaborator Author

warpfork commented Oct 1, 2020

... I think this is solid enough now, actually. If there's stuff to fix, we can do fixup commits. I'm gonna merge this now. (In part also because I want to have the prettyprinter gadget I just made come downstream of this, and use the addition of examples for this feature as part of the prettyprinter PR's proof-of-concept. :))

@warpfork warpfork merged commit f978745 into master Oct 1, 2020
@warpfork warpfork deleted the fluent-reflect branch October 1, 2020 14:34
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
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