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

exporter maxDepth #197

Merged
merged 10 commits into from
Nov 12, 2017
Merged

exporter maxDepth #197

merged 10 commits into from
Nov 12, 2017

Conversation

pgte
Copy link
Contributor

@pgte pgte commented Nov 10, 2017

In order to be able to efficiently do an ipfs.files.ls, and for it to be correct, supporting not only dirs but also shardeddirs, the best way IMO is to integrate this into the exporter, introducing an optional maxDepth option.

To fully make this happen:

  • - major exporter overhaul, concentrating dag resolving (to be able to control recursion)
  • - code to support maxDepth
  • - test maxDepth

@ghost ghost assigned pgte Nov 10, 2017
@ghost ghost added the status/in-progress In progress label Nov 10, 2017
@pgte pgte changed the title WIP: exporter maxDepth exporter maxDepth Nov 10, 2017
@pgte pgte requested a review from daviddias November 10, 2017 18:00
test/exporter.js Outdated
)
})
)
}).timeout(30 * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put timeouts at the beginning of the test

test/importer.js Outdated
@@ -311,7 +311,7 @@ module.exports = (repo) => {
done()
})
)
})
}).timeout(60 * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put timeouts at the beginning of the test

test/importer.js Outdated
@@ -331,7 +331,7 @@ module.exports = (repo) => {
done()
})
)
})
}).timeout(60 * 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put timeouts at the beginning of the test

daviddias
daviddias previously approved these changes Nov 10, 2017
Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM, just needs some timeouts increased to make all CIs happy

@@ -91,7 +91,7 @@ module.exports = (repo) => {
expect(nodes[0].path).to.be.eql(expectedHash)
expect(mh.toB58String(nodes[0].hash)).to.be.eql(expectedHash)
expect(nodes[1].path).to.be.eql(expectedHash + '/b')
expect(nodes[1].size).to.be.eql(21)
expect(nodes[1].size).to.be.eql(29)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to change if the test is the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change this for ls because go-ipfs reports the size as reported by the link, not the what we get from the file.size accessor... Still have to investigate which one is the correct one. Any clues on why these values differ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I believe that was added for ipfs.add and it seems it also made it to .ls. LGTM then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@daviddias
Copy link
Contributor

daviddias commented Nov 11, 2017

image

When using this.timeout, you have to change the arrow functions to regular function declarations

@pgte pgte mentioned this pull request Nov 12, 2017
5 tasks
@pgte
Copy link
Contributor Author

pgte commented Nov 12, 2017

Appveyor is failing with a weird permission error. Not sure if critical..
Other than that I should be all done here. @diasdavid

@daviddias daviddias merged commit 211e4e3 into master Nov 12, 2017
@daviddias daviddias deleted the feat/no-recurse branch November 12, 2017 11:39
@ghost ghost removed the status/in-progress In progress label Nov 12, 2017
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.

2 participants