-
Notifications
You must be signed in to change notification settings - Fork 26
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
add copy dagservice function #41
Conversation
daghelpers.go
Outdated
} | ||
links := node.Links() | ||
if len(links) == 0 { | ||
n := node.Copy() |
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.
FYI, you don't actually need to call Copy
on the node itself. We use that when editing nodes (i.e., Copy
then edit).
daghelpers.go
Outdated
to.Add(ctx, n) | ||
return nil | ||
} | ||
for _, link := range links { |
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.
We still need to copy the intermediate nodes, right? This just copies the leaves.
daghelpers_test.go
Outdated
) | ||
|
||
func TestCopy(t *testing.T) { | ||
ctx, cancel := context.WithCancel(context.Background()) |
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.
Should probably test with a dag that has children.
Hey @Stebalien , thx for your advice.I have update my pr.please help me review it? |
daghelpers.go
Outdated
if err != nil { | ||
return err | ||
} | ||
err = to.Add(ctx, node) |
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.
We don't need to do this once per link, just once.
daghelpers_test.go
Outdated
"context" | ||
"github.com/ipfs/go-cid" | ||
mh "github.com/multiformats/go-multihash" | ||
"github.com/stretchr/testify/assert" |
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.
We try not to add too many external deps unless we need them. I'd prefer it if we could stick with go's testing tools.
@Stebalien hello,thank u very much.And i have update pr.drop len(links) == 0 judge.please help me review again,thx a lot |
Looks good. We can improve the performance a bit (e.g., deduplicate identical sub-graphs) but this is a good start for now. |
feature #40