-
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
fix(selectors): slice matcher fixes & tests #529
Conversation
I've also removed the hard error in the slice matcher when it reaches a node that's not a string or bytes; this is more consistent with approaches of most of the other selector pieces, they just silently ignore non-matching nodes. |
55f3898
to
319ff19
Compare
Ready for review now, supports basic byte ranges in Trustless Spec now as this is, sans negative ranges (next PR will add that). Summary:
|
319ff19
to
43637d8
Compare
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.
LGTM
acc += len(byts) | ||
continue | ||
} | ||
n := copy(p, byts[mbnrs.offset-acc:]) |
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.
I dunno if this matters, but this fixture doesn't quite conform to the io.Reader interface, unless i'm mistaken. https://pkg.go.dev/io#Reader
Read is supposed to read between 0 and len(p) bytes. But if len(byts[mbnrs.offset-acc:]) > len(p) that won't be the case.
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.
copy
returns the number of bytes copied which is "the minimum of len(src)
and len(dst)
", so it's always going to be capped by len(p)
in this code and we take the returned n
as the authoritative offset increment. So I don't believe we're breaking the Reader
contract here (unless I'm mistaken!).
Draft for now; this is fine but I need to make sure there's nothing else needed here for this piece of byte range queries (there's also more pieces).readerat
so we don't over-all on the underlying read seeker