signaling ADLs in selectors #301

willscott · 2021-11-15T17:33:52Z

Provide the ExploreInterpretAs selector for specifying an ADL
Allow linksystem to hold a registry of known ADL reifiers
Tooling for registration of ADLs?
Testing
Validating all traversal paths are covered.

traversal/walk.go

traversal/selector/exploreInterpretAs.go

traversal/walk.go

willscott · 2021-12-20T09:07:24Z

Added a test. I think this should be ready for review

rvagg

🤷 it seems that all of the approaches to the ADL signalling problem that we've come up with have a smell about them, the main real benefit of this one is that there's actual code to do it. So maybe we just move the discussion forward by trying this out and seeing if its limitations are acceptable and leaning from it.

mvdan

Sounds good to me. I'll leave it to @warpfork to give the final seal of approval, as I'm not fully up to speed with the design and plans for the selector language and API.

traversal/walk_test.go

traversal/walk.go

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>

…o feat/adlselection

warpfork · 2022-01-10T14:58:32Z

traversal/selector/fieldKeys.go

@@ -21,5 +22,6 @@ const (
 	SelectorKey_LimitNone            = "none"
 	SelectorKey_StopAt               = "!"
 	SelectorKey_Condition            = "&"
+	SelectorKey_As                   = "c"


I realize this would also be a specs change, since we merged that in the ipld/ipld repo already, but since this will be the first implementation actually using it --

I still think we should change this key. There's no way I would either look at the spec and expect it to use the constant "c" here, nor look at the constant "c" in a message and guess remotely correctly what it stands for. (To wit: I just had to look it up again.)

Can we use "as"? I don't think a second byte is going to bother anyone, and my surprise factor would go down drastically.

i'm very scared to go back to step one given that we're now 2 months in to a very simple change that has now missed 2 shipping deadlines because of the spec then implementation then wait long time for review approach.

ipld/ipld#170

warpfork · 2022-01-10T15:07:39Z

traversal/selector/exploreInterpretAs.go

+// Reifiable provides a feature detection interface on selectors to understand when
+// and if Reification of the datamodel.node should be attempted when performing traversals.
+type Reifiable interface {
+	NamedReifier() string
+}


(Reviewing out loud, not a change request) This interface is implemented only once and used in only one place, so I wonder if it's necessary. Since the set of types for the compiled selectors[^1] is considered closed, it seems like we could just as well use a concrete type switch?

I'm okay leaving it this way too, because it certainly works, and I don't think it gets in the way, and perhaps someone has an idea that there will be more than one selector clause in the future which asks for a reification to be performed.

[^1] - (I wish we had done #236 by now so I could refer to what I mean by selectorengine.*...)

warpfork

Error message typo fixes.

Also tried to push them to slightly more self-similarity while at it (but honestly this whole package needs an error consistency passover, somewhat badly).

traversal/selector/exploreInterpretAs.go

Co-authored-by: Eric Myhre <hash@exultant.us>

…election

…o feat/adlselection

warpfork · 2022-01-10T15:40:17Z

Replaying a conversation that @willscott and I had elsewhere, for posterity:

My initial thought was to hope we could put most logic like this -- anything that wants an ADL naming table -- into the middle of a NodeReifier callback, and thus push off having strong opinions about it just a little while longer. (While, nonetheless, expecting something semistandard to still be able to emerge over time.) This would also keep the linking package from having to know anything at all about ADLs, which would be pleasing.

This turns out not to fly together with this idea of Selectors carrying ADL signalling information. NodeReifier doesn't get information about Selectors.

Putting the ADL naming table in the LinkSystem struct forces us to start making more standards about the naming. But means the traversal code can have the knowledge of the Selectors and that naming table at the same time, and thus accomplish the goal of letting information in Selectors do the ADL signalling.

Perhaps a third option would've been adding a field to linking.LinkContext that allows carrying the selector there. Then, NodeReifier callbacks would've gotten access to the info needed. I'm not sure we considered this earlier. It seems dubious though, because teaching the linking package about selector... I'm not sure if that would compile. Maybe it even would. But it still seems like it would be rather more of an arc through the dependency graph than I'd expect the linking package to have.

So we end up with this.

I'm a little nervous about getting a map in the LinkSystem type, when we've avoided that for every other thing LinkSystem needs to wrangle so far, but all the other indirections examined so far don't seem better, and we seem to have run out of patience for noodling about it any further right now. Alright. 🚢

warpfork

Alright, let's go

warpfork · 2022-01-10T15:56:48Z

Future wish: more test fixtures that exercise this feature in a data-driven way and live in the ipld/ipld repo.

I don't feel able to hold folks up on that at the moment, but it would probably be easier to review those, and also we'll need them if we expect other languages and other implementations to be able to confidently match this implementation.

- The prefix "Explore" on its name feels unnecessary; drop. (Doesn't change the protocol structurally.) - The 'as' field is no longer renamed to 'c' in the serial form. (DOES change the protocol structurally; but there's only one implementation of this, and we changed it too. See ipld/go-ipld-prime#301 (comment).) - Added a bunch of docs.

warpfork · 2022-03-08T23:57:01Z

(Followup note: we actually got fixtures that happen to exercise this, recently: they appeared as part of the specification for range clauses for bytes in ipld/ipld#184. Fabulous!)

first pass at signaling ADLs

ed1d924

willscott requested a review from warpfork November 15, 2021 17:33

mvdan reviewed Nov 17, 2021

View reviewed changes

willscott added 3 commits December 12, 2021 10:28

Merge remote-tracking branch 'origin/master' into feat/adlselection

e8b780d

Respond to code review

2e6e1f7

add test for exploreInterpretAs

da0b8e6

rvagg approved these changes Jan 4, 2022

View reviewed changes

mvdan approved these changes Jan 4, 2022

View reviewed changes

traversal/walk_test.go Outdated Show resolved Hide resolved

traversal/walk.go Show resolved Hide resolved

traversal/walk.go Outdated Show resolved Hide resolved

willscott and others added 4 commits January 10, 2022 00:31

Update traversal/walk_test.go

6f3ea54

Co-authored-by: Daniel Martí <mvdan@mvdan.cc>

better error wrapping

5312b3e

Merge branch 'feat/adlselection' of github.com:ipld/go-ipld-prime int…

bef2a38

…o feat/adlselection

consistent test case

ccb336c

warpfork reviewed Jan 10, 2022

View reviewed changes

warpfork requested changes Jan 10, 2022

View reviewed changes

traversal/selector/exploreInterpretAs.go Outdated Show resolved Hide resolved

traversal/selector/exploreInterpretAs.go Outdated Show resolved Hide resolved

willscott and others added 5 commits January 10, 2022 07:26

Update traversal/selector/exploreInterpretAs.go

aba58f1

Co-authored-by: Eric Myhre <hash@exultant.us>

Update traversal/selector/exploreInterpretAs.go

73fc35f

Co-authored-by: Eric Myhre <hash@exultant.us>

Merge branch 'master' of github.com:ipld/go-ipld-prime into feat/adls…

0b2f841

…election

Merge branch 'feat/adlselection' of github.com:ipld/go-ipld-prime int…

f97c150

…o feat/adlselection

update fieldkey per @warpfork request

838d340

warpfork approved these changes Jan 10, 2022

View reviewed changes

willscott merged commit 5e33eba into master Jan 10, 2022

willscott deleted the feat/adlselection branch January 10, 2022 15:42

warpfork mentioned this pull request Jan 10, 2022

selectors: touch up InterpretAs ipld/ipld#170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signaling ADLs in selectors #301

signaling ADLs in selectors #301

willscott commented Nov 15, 2021 •

edited

Loading

willscott commented Dec 20, 2021

rvagg left a comment

mvdan left a comment

warpfork Jan 10, 2022

willscott Jan 10, 2022

warpfork Jan 10, 2022

warpfork Jan 10, 2022

warpfork left a comment

warpfork commented Jan 10, 2022

warpfork left a comment

warpfork commented Jan 10, 2022

warpfork commented Mar 8, 2022

signaling ADLs in selectors #301

signaling ADLs in selectors #301

Conversation

willscott commented Nov 15, 2021 • edited Loading

willscott commented Dec 20, 2021

rvagg left a comment

Choose a reason for hiding this comment

mvdan left a comment

Choose a reason for hiding this comment

warpfork Jan 10, 2022

Choose a reason for hiding this comment

willscott Jan 10, 2022

Choose a reason for hiding this comment

warpfork Jan 10, 2022

Choose a reason for hiding this comment

warpfork Jan 10, 2022

Choose a reason for hiding this comment

warpfork left a comment

Choose a reason for hiding this comment

warpfork commented Jan 10, 2022

warpfork left a comment

Choose a reason for hiding this comment

warpfork commented Jan 10, 2022

warpfork commented Mar 8, 2022

willscott commented Nov 15, 2021 •

edited

Loading