Selector explore implementation #402
Conversation
austinabell
requested review from
StaticallyTypedAnxiety,
flodesi,
RajarupanSampanthan and
timvermeulen
There was a problem hiding this comment.
Looks good Austin! Understandable that you closely follow the Go implementation, though the interests/explore setup seems a bit convoluted on first readthrough. I think we'll probably end up with more clones (of Selector) than necessary because of this. Might be interesting to experiment with this after implementation/testing is done.
Yeah that was what I mentioned I was going to do, I don't like the current setup but going to wait until after full functionality tests before refactoring. I have some ideas but counterproductive to make now since we need to match the specific implementation in go |
| pub fn explore(self, _ipld: &Ipld, _p: &PathSegment) -> Option<Selector> { | ||
| // TODO | ||
| todo!() | ||
| pub fn explore(self, ipld: &Ipld, p: &PathSegment) -> Option<Selector> { |
There was a problem hiding this comment.
This looks great, I only had a question on is selector a heavy enum? If the answer is yes what do you think about a reference?
It would also be interested(although abit premature right now) to see selector implement an iterator so that we could use it more lazily
There was a problem hiding this comment.
Not quite sure what you mean by heavy enum, care to elaborate? Yeah it would definitely be a more idiomatic interface to have this as an iterator, but I will hold off until the functionality is fully tested (in next PR). Great suggestion!
There was a problem hiding this comment.
Should have been more clear sorry! Something that occupies a lot of data, I guess what I am getting at is asking if the clones are going to be expensive but then again that is also a premature optimization
There was a problem hiding this comment.
Yeah, in practice it'll be very small (depends on the construction of the selector) but since it's recursive, there is no cap on the size.
Unfortunately the clones are intended (as far as I can see) because of the certain branches which would modify selectors with requiring the previous to not be mutated. This is something I will explore very in depth to make sure there is not a more efficient way around this once I have a testsuite verifying all functionality.
Summary of changes
Changes introduced in this pull request:
I wrote the tests in json with a runner to be able to test against go impl and provide a language agnostic test suite (I believe JS selectors are in progress rn too)
#241 for reference to spec and go impl
Reference issue to close (if applicable)
Closes
Other information and links