-
Notifications
You must be signed in to change notification settings - Fork 50
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
Changes from 4 commits
ed1d924
e8b780d
2e6e1f7
da0b8e6
6f3ea54
5312b3e
bef2a38
ccb336c
aba58f1
73fc35f
0b2f841
f97c150
838d340
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
package selector | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/ipld/go-ipld-prime/datamodel" | ||
) | ||
|
||
type ExploreInterpretAs struct { | ||
next Selector // selector for element we're interested in | ||
adl string // reifier for the ADL we're interested in | ||
} | ||
|
||
// Interests for ExploreIndex is just the index specified by the selector node | ||
func (s ExploreInterpretAs) Interests() []datamodel.PathSegment { | ||
return s.next.Interests() | ||
} | ||
|
||
// Explore returns the node's selector if | ||
// the path matches the index for this selector or nil if not | ||
func (s ExploreInterpretAs) Explore(n datamodel.Node, p datamodel.PathSegment) (Selector, error) { | ||
return s.next, nil | ||
} | ||
|
||
// Decide always returns false because this is not a matcher | ||
func (s ExploreInterpretAs) Decide(n datamodel.Node) bool { | ||
return false | ||
} | ||
|
||
// NamedReifier indicates how this selector expects to Reify the current datamodel.Node. | ||
func (s ExploreInterpretAs) NamedReifier() string { | ||
return s.adl | ||
} | ||
|
||
// 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 | ||
} | ||
|
||
// ParseExploreInterpretAs assembles a Selector | ||
// from a ExploreInterpretAs selector node | ||
func (pc ParseContext) ParseExploreInterpretAs(n datamodel.Node) (Selector, error) { | ||
if n.Kind() != datamodel.Kind_Map { | ||
return nil, fmt.Errorf("selector spec parse rejected: selector body must be a map") | ||
} | ||
adlNode, err := n.LookupByString(SelectorKey_As) | ||
if err != nil { | ||
return nil, fmt.Errorf("selector spec parse rejected: fields in ExploreFields selector must be present") | ||
willscott marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
next, err := n.LookupByString(SelectorKey_Next) | ||
if err != nil { | ||
return nil, fmt.Errorf("selector spec parse rejected: next field must be present in ExploreAll selector") | ||
willscott marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
selector, err := pc.ParseSelector(next) | ||
if err != nil { | ||
return nil, err | ||
} | ||
adl, err := adlNode.AsString() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return ExploreInterpretAs{selector, adl}, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ const ( | |
SelectorKey_ExploreUnion = "|" | ||
SelectorKey_ExploreConditional = "&" | ||
SelectorKey_ExploreRecursiveEdge = "@" | ||
SelectorKey_ExploreInterpretAs = "~" | ||
SelectorKey_Next = ">" | ||
SelectorKey_Fields = "f>" | ||
SelectorKey_Index = "i" | ||
|
@@ -21,5 +22,6 @@ const ( | |
SelectorKey_LimitNone = "none" | ||
SelectorKey_StopAt = "!" | ||
SelectorKey_Condition = "&" | ||
SelectorKey_As = "c" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// not filling conditional keys since it's not complete | ||
) |
There was a problem hiding this comment.
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.*
...)