-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement Resolver iterator #741
Conversation
07bd42f
to
6072cdd
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.
First of all, thank you for the contribution! The code looks good, but there are few places that need to be improved :)
graph/iterator/resolver.go
Outdated
nodes: make([]quad.Value, 0, 20), | ||
} | ||
// Enforce uniqueness | ||
unique := make(map[quad.Value]bool, 20) |
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 would actually expect it to iterate over duplicates. It might resolve duplicates once, but in general, it should preserve an order and the number of values.
graph/iterator/resolver_test.go
Outdated
|
||
func TestResolverIteratorIterate(t *testing.T) { | ||
var ctx context.Context | ||
qs, err := cayley.NewMemoryGraph() |
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.
Requiring the top-level cayley
package from low-level iterators
package is a bit an overkill. Please check other iterator tests, there should be a mock store for such tests.
package iterator_test | ||
|
||
import ( | ||
"testing" |
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.
running goimports
over it will be nice (analog of gofmt
)
6072cdd
to
4d5578d
Compare
4d5578d
to
07ef7be
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.
Looking good, almost there :)
graph/iterator/resolver.go
Outdated
func (it *Resolver) Contains(ctx context.Context, value graph.Value) bool { | ||
graph.ContainsLogIn(it, value) | ||
if !it.cached { | ||
it.resolve(ctx) |
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.
It should set it.err
, same for Next
graph/iterator/resolver.go
Outdated
// Next advances the iterator. | ||
func (it *Resolver) Next(ctx context.Context) bool { | ||
graph.NextLogIn(it) | ||
if it.index == len(it.order) { |
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.
>=
, just to be on the safe side
Implement an iterator that takes a slice of nodes and an associated QuadStore and resolves to their respective values during iteration. Resolves cayleygraph#663
07ef7be
to
9ff9edc
Compare
@phyrwork There are only a few small changes left. Do you have time to finish it? Will be happy to merge it. |
Nevermind, I fixed and merged it. Thanks for contributing! |
Implement an iterator that takes a slice of nodes and an
associated QuadStore and resolves to their respective
values during iteration.
Resolves #663