Add a demo ADL (rot13adl) #98
Conversation
|
https://github.com/ipld/go-ipld-prime/pull/98/checks?check_run_id=1280174810 appears to be failed because of some kind of intermittent error in the backends -- something about "Failed to download action". The tests would surely pass otherwise, since all the other variations of environments do. |
warpfork
force-pushed
the
adl-demo-rot13
branch
from
bdb19d9
to
b1e837b
Compare
| SubstrateRoot _Substrate__Prototype | ||
| } | ||
|
|
||
| // REVIEW: In ADLs that use codegenerated substrate types defined by an IPLD Schema, the `Prototype.SubstrateRoot` field... |
There was a problem hiding this comment.
Here's another of the big review questions: what should we expect about substrate nodes and their prototypes and builders? (... In ADLs that export some! -- remember, this is not strictly mandated; ADLs can be implemented using just basicnode in their guts too.)
There's some disharmony with how schema typed nodes currently treat this. I'm not sure in which direction this should be resolved -- by moving the ADLs closer to how typed nodes behave? Or by changing how schema nodes behave to be more like ADLs, returning the same form you started with and making you take another step to get high level views out?
adl/rot13adl/rot13node_test.go
Outdated
| // TODO: the cast above isn't available outside this package: we should probably make an interface with `Substrate()` and make it available. | ||
| // Is it reasonable to make this part of a standard feature detection pattern, | ||
| // and make that interface reside in the main IPLD package? Or in an `adl` package that contains such standard interfaces? |
There was a problem hiding this comment.
Here's another place where it seems like a consistent interface for detecting ADLs might be useful. What do with think of this? yes/no/maybe?
There was a problem hiding this comment.
I'm gonna go with "maybe", because I agree that this should be consistent and easy between ADLs, but I don't like the name "substrate" :)
How about UnderlyingNode? Or if you want to clarify that it's just for ADLs, perhaps something like UnderlyingDataLayout.
There was a problem hiding this comment.
So I have to confess this particular suggestion of alternative name made me laugh because of personal history.
(For whatever reason, in some of my earliest programming projects, I really latched onto the word "underlying". I overused it horribly, everywhere. At some point I became actively self-conscious of it and started just stripping the word out of the sentence every time I was tempted to use it, just like I try to strip "simply" nowadays.)
I don't want to say it's an objectively bad word, because it isn't, but this just gave me a hoot.
There was a problem hiding this comment.
A little more justifiably: I think I prefer "substrate" to "underlying" because "substrate" sounds a bit more like it's a precise term-of-art, and I think it'll be clearer to write docs and specs which strongly associate that word to ADLs. By contrast, "underlying" is an adjective that could be applied to several parts of IPLD: e.g., one could also say "typed nodes have both a typed interface and an underlying representation interface" and it would seem to make sense; this means it's a little harder to bake that word into a term-of-art.
There was a problem hiding this comment.
That sounds fine, but then we have to define "substrate" in the specs/docs early on, which I don't think we do right now.
| nb := rot13adl.Prototype.SubstrateRoot.NewBuilder() | ||
|
|
||
| // Unmarshal -- using the substrate's nodebuilder just like you'd unmarshal with any other nodebuilder. | ||
| err := dagjson.Unmarshal(nb, json.NewDecoder(strings.NewReader(`"n pbby fgevat"`))) |
There was a problem hiding this comment.
now there's a neat trick! I'll borrow that one
| // REVIEW: this function is currently not conforming to any particular interface; | ||
| // if we evolve the contract for ADLs to include an interface for reficiation functions, | ||
| // might we need to add context and link loader systems as parameters to it? | ||
| // Not all implementations might need it, as per previous paragraph; but some might. | ||
| // Reification for multiblock ADLs might also need link loader systems as a parameter here | ||
| // so they can capture them as config and hold them for use in future operations that do lazy loading. |
There was a problem hiding this comment.
One of the big review comments is here. Should we try to make a standard interface for reify functions?
I haven't yet, here. But it might be useful. And having an interface type might also be useful for guidance for designers and implementers of new ADLs.
There was a problem hiding this comment.
My response here is pretty much the same as my response to Substrate :)
I really, really wish this could be just AssignNode. Maybe we could rethink that API a bit so that we can avoid NewBuilder when it's not needed. Having an extra method documented as "like AssignNode but will save you an alloc in some trivial cases" would be pretty weird.
There was a problem hiding this comment.
I wish that too, but I think in this case, we're going to end up needing a new function because it's going to be doing a lot more than dodge some allocs. The need for link loader configuration is going to make for a noticeably different function signature. It doesn't show up in this demo because the demo isn't doing multiblock stuff, but in a lot of other ADLs, it will.
(Probably. Unless we figure out some other API for shuffling in those link loader systems from the side, somehow. But I think you're as up-to-date on the thinking about this as I am, at this point...)
|
|
||
| There are several ways to move data in and out of the ADL: | ||
|
|
||
| - treat it like a regular IPLD map: |
There was a problem hiding this comment.
This probably isn't as interesting as using the substrate directly but incorporating this into the example might be nice, to show the higher-level API that will mostly be consumed by users. I suppose the example could set a string property, then Subtrate() and inspect the property underneath it?
There was a problem hiding this comment.
I see the tests exercising all of this, I think it'd be worth raising some more of that functionality up into the example since it's doing the initial work of demonstrating how this all works and you need to set up the mental model properly up front. IMO.
There was a problem hiding this comment.
Example added. Thanks for the kick.
|
my main questions around this, and noting that i agree that the machinery here is needed in some form, is how we make this work with the link-loader used by pathing / selectors |
Agree, there some slightly dizzying questions there. An ADL with a multiblock substrate, the node surely needs to already have link loader functions available to it, so it can answer any of the single-step But a traversal has its own link loader functions which are used for any links it encounters during its (high level / does-not-see-the-substrate-if-its-stepping-over-an-ADL) walk. Perhaps those two link loaders just aren't the necessary the same, and that's okay, and we just document that. If the link loader functions in those two places are different, and the library user did that by mistake instead of intention, I could see that making some pretty head-scratching bugs. But I also don't see a way to outlaw it. How this plays out in practice might also depend on how the ADL is getting noticed/created. If it's during a traversal that the ADL gets noticed in the first place (say, it's an advanced traversal (of a kind we haven't implemented yet, roll with me here) that takes a Selector as a parameter, and some of the Selector matcher clause labels say "use $foobar ADL when this matcher matches"), then it would probably make sense that we'd just copy the traversal's link loader functions into the ADL node. I think this kind of propagation of link loaders will probably "DTRT" in the vast majority of cases and should get us pretty far. (Hopefully far enough that we can defer worrying about any other needs until they're discovered later.) |
…ions about how to implement Reify; consistent export symbol conventions; some fixes.
…cessing substrate. Fixed a symbol to be exported that's needed for this to be possible when outside the package. This still probably deserves an interface, too, though. Comments on that also updated, but we'll still leave that for future work (more examples of more ADLs wanted before we try to solidify on something there).
warpfork
force-pushed
the
adl-demo-rot13
branch
from
b1e837b
to
6541383
Compare
|
Okay, I think this has reached the point of "future work? maybe. good enough? probably". I've just added a few more examples, and am going to merge as soon as CI for those goes green. (It's PR cleanup-and-finish day. Too many things open!) |
I've been working on an ADL implementation for demo purposes -- a "rot13" ADL: it treats the raw data as a rot13 string, and rotates it for the high level view. This is a pretty useless gadget... except it's so simple that I'm hoping it will make a good piece of example code, to help show what ADLs do and how their interfaces should work. The goal of this is to be able to hand a link to this package to someone wanting to implement a new ADL, and have them get a lot of useful pointers about how to start implementing their own ideas in a new package of their own.
The package docs in the diff body say more about the publicly exported interfaces and what kind of logical contracts we expect from them, but in summary:
Node/NodeBuilder/NodePrototypetypes, which expose the synthesized view of the ADL after the data has been processed;Node/NodeBuilder/NodePrototypefamily. In this case, there's only one of of them, but in more complex ADLs, there'd probably be a root type, and then the whole family again for the various other types of internal data (... if the ADL internals are specified by an IPLD Schema that is; it's not required for them to be!).Reify(maybeSubstrate Node) (synthesized Node, orNope error)method to take substrate data already in memory and get an ADL out of it.Substrate() Nodemethod on the high-level ADL Node type which lets you get at the guts inside.(You can imagine all the arrows between those modes: they should form a pretty cyclic graph -- you should be able to get from any view of the data to any other view pretty freely.)
There's a couple of design review questions I still don't have a confident final answer on; these are marked with "REVIEW" or "TODO" comments in the diff. I'll start comment threads on them so we can discuss more here. (I'm not sure all of them will need strict answers right now -- maybe some of it will be worth standardizing early; maybe some of this stuff might be totally fine developing in independent directions in various ADL projects for a while. We'll see!)