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

signaling ADLs in selectors #301

Merged
merged 13 commits into from
Jan 10, 2022
Merged

signaling ADLs in selectors #301

merged 13 commits into from
Jan 10, 2022

Conversation

willscott
Copy link
Member

@willscott willscott commented Nov 15, 2021

  • 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 Outdated Show resolved Hide resolved
traversal/walk.go Outdated Show resolved Hide resolved
traversal/walk.go Outdated Show resolved Hide resolved
traversal/selector/exploreInterpretAs.go Outdated Show resolved Hide resolved
traversal/walk.go Outdated Show resolved Hide resolved
@willscott
Copy link
Member Author

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

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

🤷 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.

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.

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 Outdated Show resolved Hide resolved
traversal/walk.go Show resolved Hide resolved
traversal/walk.go Outdated Show resolved Hide resolved
@@ -21,5 +22,6 @@ const (
SelectorKey_LimitNone = "none"
SelectorKey_StopAt = "!"
SelectorKey_Condition = "&"
SelectorKey_As = "c"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +35 to +39
// 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
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

(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.*...)

Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
traversal/selector/exploreInterpretAs.go Outdated Show resolved Hide resolved
@warpfork
Copy link
Collaborator

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. 🚢

Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

Alright, let's go

@willscott willscott merged commit 5e33eba into master Jan 10, 2022
@willscott willscott deleted the feat/adlselection branch January 10, 2022 15:42
@warpfork
Copy link
Collaborator

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.

warpfork added a commit to ipld/ipld that referenced this pull request Jan 10, 2022
- 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
Copy link
Collaborator

warpfork commented Mar 8, 2022

(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!)

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.

4 participants