go-ipld-prime
go-ipld-prime copied to clipboard
ipld
fluent: consider deprecating and then removing Reflect APIs in favor of bindnode
bindnode is also implemented via reflection, it has more features (such as using custom Go types), and I believe it supports all the features that these fluent reflect APIs provide.
We should probably deprecate the fluent reflect APIs (n.b. just the "reflect" family, not all of the fluent package) in favor of bindnode, and remove them a couple of releases later.
Before doing any of this, we could be nice and search across PL for users of these APIs, and submit PRs to move them to bindnode.
When we deprecate, we can then also close https://github.com/ipld/go-ipld-prime/issues/212.
cc @warpfork @rvagg
func Reflect(np datamodel.NodePrototype, i interface{}) (datamodel.Node, error)
This would be replaced by bindnode.Wrap, most of the time.
One quirk is that bindnode represents maps as a keys-values tuple, to maintain map order. The fluent APIs deal with this by sorting keys via Reflector.MapOrder. We should keep this in mind when updating existing bits of code. I imagine that some of them could simply replace a Go map with a Go struct, for instance.
func ReflectIntoAssembler(na datamodel.NodeAssembler, i interface{}) error
I don't imagine many people will be using this today. But one could certainly use bindnode.Wrap with a custom prototype to gain more control.
Hm. Sounds potentially legit to me.
I'd like to spend a moment wondering out loud if it will have any effect on maintainability, and performance, feature range, and the library's explanatory effect.
Maintenance: seems neutral: these reflect functions haven't really had any maintenance costs since first penned; bindnode does (at least for now, as it continues to grow in range of what it supports), but it's so unquestionably valuable that those investments are already accounted for before this.
Performance: bindnode is going to spend some time inferring a schema, rather than directly proceeding through the very simple switch table that fluent.Reflect uses. I'm not sure exactly how much that costs. That said... I imagine if one is using these reflect functions at all, one is unlikely to be ranking speed absolute first (these methods have always disclaimed that's not what they prioritize), so, surely it's fine. Also, as bindnode gets performance work, having the results also 'for free' here would seems nice.
Feature range: that's easy, bindnode already eclipses these functions, completely, I think.
Library's explanatory effect: okay, this is in the eye of the beholder; and it also may not be worth prioritizing here. That disclaimer aside: something I appreciate about fluent.Reflect is that it's so simple that it's linearly readable, and could be pointed at as part of learning material about how a developer can mentally map golang types vs the IPLD Data Model. This is probably worth dismissing, though: if we really wanted this to be discoverable and useful as learning material, we can replicate that code in a documentation site or such.
Overall: yep, I can't find any objection.
We may want to leave some function stubs even beyond a deprecation period, which redirect to the bindnode behaviors? It may help keep guiding people to the right place if they look in this package for these feature. (go-ipld-prime/node/bindnode, from the package name alone, isn't super likely to jump out at a new user looking for these features.)
On the other hand, such facade and discovery aid functions could just as well belong in the root package, nowaday, too.
I thought about forwarding APIs as well. We could certainly do that for backwards compatibility in the fluent package. I'm less certain that a top-level reflect API will be a good idea; it's either going to be a copy of the bindnode API, in which case we duplicate, or it's going to be a simplified version, at the cost of either features or performance.
Yeah, I think I'm fine with this. I had an instinctive negative reaction to this but it's mostly about the simple cases, like building data for tests. But mostly I think that's just an aversion to having to introduce types for even the simplest tasks. And also, when you get simple enough you can also just use basicnode, which I think I'd reach for. So no, I don't think I can see myself reaching for the fluent APIs for anything new at the moment.
