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

End export stream on completion. #47

Merged
merged 4 commits into from
Jun 28, 2016

Conversation

hackergrrl
Copy link
Contributor

@hackergrrl hackergrrl commented Jun 7, 2016

I noticed that the Exporter stream doesn't actually end. This PR fixes that and also adds tests that removes this assumption, and also some timing assumptions. It also cleans up the overall DAG traversal logic.

I also switched us over to concat-stream from bl, just because the former handles object streams nicely too.

@hackergrrl
Copy link
Contributor Author

@diasdavid @nginnever ping for cr

@hackergrrl hackergrrl force-pushed the end-exporter-stream branch from 1d9f043 to 879f9a8 Compare June 15, 2016 00:42
@hackergrrl
Copy link
Contributor Author

This also includes directories in the Exporter. Previously they were omitted (unless the dir had no links), which is unlike go-ipfs, which includes the directories every time. I can split this change into a separate PR if it's easier to review!

@hackergrrl hackergrrl force-pushed the end-exporter-stream branch from 38063d1 to 461c500 Compare June 15, 2016 02:05
@hackergrrl
Copy link
Contributor Author

ping @diasdavid @nginnever

Readable.call(this, { objectMode: true })

this.options = options || {}

this._read = (n) => {}

let fileExporter = (node, name, callback) => {
let fileExporter = (node, name, done) => {
let init
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we initialize this to false so that L47 and L59 can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done.

@nginnever
Copy link
Contributor

nginnever commented Jun 23, 2016

This LGTM, just a few little comments. I also tried testing this in js-ipfs cat and get cli commands and cat seems to still function fine but get is failing again, but not due to the PR, it's another bug that I need to fix. get cli command will also need compression options too so it needs to be revisited anyway.

@daviddias
Copy link
Contributor

daviddias commented Jun 28, 2016

This looks good, having the hash prepended kind of grew on me :) Thank you!

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.

3 participants