Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add: Fix adding multiple files #6255

Merged
merged 3 commits into from
Apr 25, 2019
Merged

add: Fix adding multiple files #6255

merged 3 commits into from
Apr 25, 2019

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Apr 25, 2019

See #6254

magik6k added 3 commits April 25, 2019 15:03
@magik6k
add: fix adding multiple files
0b9e89d 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
add: fix wrap with multiple files
aa0704e 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
add: test adding multiple files
ac51d29 
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@ghost ghost assigned magik6k Apr 25, 2019
@ghost ghost added the status/in-progress In progress label Apr 25, 2019
@magik6k magik6k mentioned this pull request Apr 25, 2019
Closed
@magik6k magik6k requested a review from Stebalien April 25, 2019 13:56
Stebalien
Stebalien approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd like to reconsider how we're doing events (ideally, we'd be able to reuse the channel) but we can handle that in a followup PR.

core/commands/add.go
_, dir := addit.Node().(files.Directory)
errCh := make(chan error, 1)
events := make(chan interface{}, adderOutChanSize)
opts[len(opts)-1] = options.Unixfs.Events(events)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we shouldn't be closing this channel?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we could drop the need for the nil option by writing:

opts := append(opts, options.Unixfs.Events(events)) // note the :=

@Stebalien Stebalien merged commit 5d9651a into master Apr 25, 2019
@Stebalien Stebalien deleted the fix/add-many branch April 25, 2019 16:32
@ghost ghost removed the status/in-progress In progress label Apr 25, 2019
@hugomrdias hugomrdias mentioned this pull request May 6, 2019
Merged
@magik6k magik6k mentioned this pull request May 14, 2019
Closed
@Kubuxu Kubuxu mentioned this pull request May 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

Assignees

@magik6k magik6k

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@magik6k @Stebalien