-
Notifications
You must be signed in to change notification settings - Fork 53
feat(Directory): Add EnumLinksAsync method #39
Conversation
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'll take a closer look at the end of the week (left some comments for now) but this makes plenty of sense. 👍
io/directory.go
Outdated
go func() { | ||
defer close(linkResults) | ||
for _, l := range d.node.Links() { | ||
linkResults <- format.LinkResult{ |
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.
This should also select on ctx to avoid leaking goroutines (same in other places)
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.
got it. will do.
cfc13dc
to
6ec34f8
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.
Few more nits, then LGTM
hamt/hamt.go
Outdated
"github.com/spaolacci/murmur3" | ||
|
||
format "github.com/ipfs/go-unixfs" |
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 be above this group
io/directory.go
Outdated
@@ -141,6 +146,26 @@ func (d *BasicDirectory) AddChild(ctx context.Context, name string, node ipld.No | |||
return d.node.AddNodeLink(name, node) | |||
} | |||
|
|||
// EnumLinksAsync returns a channel which will receive Links in the directory | |||
// as they are enumerated, where order is not gauranteed | |||
func (d *BasicDirectory) EnumLinksAsync(ctx context.Context) (<-chan format.LinkResult, error) { |
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'd drop the error from the signature since it's not used here (or other impls of this function)
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.
fair
- Add LinkResult type to unix-fs - is an ipld Link or an error - Add EnumLinksAsync method to Directory interface, returns channel of directory links or error - Add EnumLinksAsync method to Shard interface in HAMT, returns channel of directory links or error - EnumLinks method in Shard interface in HAMT uses EnumLinksAsync now - modify makeAsyncTrieGetLinks to use channel
eee76dd
to
c54d0e4
Compare
@schomatis are you good with my responses? Can we go ahead and merge this PR? |
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, I have one follow-up question but if this is blocking something else go ahead and merge.
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 module the @schomatis comment (not blocking for me)
I think I'm going to go ahead and merge but feel free to continue this discussion. |
…5600 feat(Directory): Add EnumLinksAsync method This commit was moved from ipfs/go-unixfs@a3eae7f
Goal
child of ipfs/kubo#5600
Implement an EnumLinksAsync method on Directory interface to support streaming version of
ls
commandImplementation
of directory links or error
For Discussion
ls
to go-ipfs