search

ipld / go-ipld-adl-hamt

Other
4 stars 4 forks source link

Update to newer Reify convention #30

Closed willscott closed 2 years ago

willscott commented 3 years ago

Presumably the current global Reify in this repo that's just a TODO should get updated to fit the NodeReifier type

mvdan commented 3 years ago

Unclear to me - this Reify API came from @warpfork's original skeleton. I don't think reification needs a linkcontext and linksystem?

willscott commented 3 years ago

If you want to be able to have this integrated into the linksystem so that links that should be re-ified are, then you'd presumably still want the newer interface.

Separately, we'll need a meta-reifier that attempts the various different ADLs, or can learn and set them appropriately based on pathing (the signalling problem)

warpfork commented 3 years ago

Yeah, unclear if the Reify method here should fit LinkSystem.NodeReifier. Leaning towards no.

LinkSystem.NodeReifier has the responsibility for choosing when and where to use an ADL. (e.g., yeah, it's where one plugs in one's solution to the signalling problem.)

Reify in this package is for using imperatively as a library when you've already decided that you've got a hamt.

So, one might see Reify of this package called within a LinkSystem.NodeReifier function. But I think it's rare-to-never that one would want to plug Reify in directly to a LinkSystem.NodeReifier, because that would be saying "try to turn every single thing you see into a HAMT", which is... probably not gonna be correct.

willscott commented 3 years ago

Our other example of an ADL make use of the NodeReifier interface: https://github.com/ipfs/go-unixfsnode/blob/main/reification.go#L16

If some seem to think that's natural, shouldn't we just standardize on it? the wider method signature doesn't seem to hurt, and i'd prefer one signature rather than multiple

mvdan commented 3 years ago

I'm fine to go with the flow, and I agree on consistency. I just happen to agree with Eric that the standard API for ADLs should be the shorter func signature :) The longer func signature could be part of a "multi-ADL" package that knows how to register and use many ADL implementations.

rvagg commented 2 years ago

going to close this and say #35 takes over the discussion