Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

files.get #271

Closed
wants to merge 2 commits into from
Closed

files.get #271

wants to merge 2 commits into from

Conversation

nginnever
Copy link
Contributor

@xicombd

I added the compression args to what you had worked on earlier. Not sure if this is the most elegant way to handle these things so let me know what you think! :D

@nginnever nginnever mentioned this pull request May 13, 2016

module.exports = (send) => {
return function get (path, archive, compress, compressionLevel, cb) {
if (archive === true && typeof compress === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use a switch statement instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or since archive, compress, compressionLevel are all optional, why not pass them on the opts object?

I think that you can just return the argCommand and it will take care of that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, please make archive, compress and compressionLevel into an options object.

@daviddias
Copy link
Contributor

awesome @nginnever, great start! :D get should offer the same API as js-ipfs, as you know, it returns an event for each file that is transmitted.

Also, you should include tests for 'get directories'

.on('data', (data) => {
buf += data
})
.on('end', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use bl for these tests

@daviddias daviddias changed the title get command files.get Jun 6, 2016
@daviddias
Copy link
Contributor

@nginnever what is the status of this PR?

@nginnever
Copy link
Contributor Author

@diasdavid still needs a lot of work. Now that the add and cat command are starting to be finalized in all of the modules this can be restarted.

Still needs unixfs-engine compression, js-ipfs update, and interface-core spec. There are some tests began here that could start to be moved to interface-core tests.

@daviddias
Copy link
Contributor

Still needs unixfs-engine compression

The compression doesn't happen at the unixs-engine level right? It is just at the wire level that everything gets tar'ed

@hackergrrl
Copy link
Contributor

Hey @nginnever! I gave this the last little bit of spit 'n shine it needed: #296

If that looks good, I think we can safely close this PR.

@daviddias
Copy link
Contributor

Followed in #296

@daviddias daviddias closed this Aug 4, 2016
@daviddias daviddias deleted the feat/get branch August 4, 2016 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants