feat: .getMany #81
feat: .getMany #81
Conversation
ghost
assigned 
|
Ready for merge and release |
test/block-service-test.js
Outdated
| expect(blocks).to.eql(blocks) | ||
| cb() | ||
| }) | ||
| expect(testBlocks).to.eql(testBlocks) |
There was a problem hiding this comment.
Shouldn't this be expect(_blocks).to.eql(testBlocks) (and then _blocks should be renamed to blocks).
test/block-service-test.js
Outdated
| expect(err).to.not.exist() | ||
| bs.getMany(cids, (err, _blocks) => { | ||
| expect(err).to.not.exist() | ||
| expect(testBlocks).to.eql(testBlocks) |
There was a problem hiding this comment.
Shouldn't this be expect(_blocks).to.eql(testBlocks) (and then _blocks should be renamed to blocks).
PS: I repost this comment as GitHub doesn't show the original one as I expect it.
src/index.js
Outdated
| @@ -94,6 +96,24 @@ class BlockService { | |||
| return this._repo.blocks.get(cid, callback) | |||
| } | |||
|
|
|||
| /** | |||
| * Get multiple blocks a block by cid. | |||
There was a problem hiding this comment.
I think this comment should've been
Get multiple blocks by cids
?
src/index.js
Outdated
| return callback(new Error('first arg must be an array of cids')) | ||
| } | ||
| if (this.hasExchange()) { | ||
| return this._bitswap.getMany(cids, callback) |
There was a problem hiding this comment.
One thing that baffled me at first sight was that this synchronously returns, while in the next few lines, we asynchronously fetch from the local repository.
After looking at Bitswap's API I noticed that getMany() isn't actually synchronous either, which makes me wonder:
Are we only returning here to stop getMany() ?
If yes, couldn't we then also go with:
this.hasExchange() ?
this._bitswap.getMany(cids, callback) :
asyncMap(cids, (cid, cb) => this._repo.blocks.get(cid, db, callback)
Then it might be more obvious that we aren't interested in returning anything here (even though getMany kind of says the opposite, but hey... it's async)
There was a problem hiding this comment.
I prefer if/else over complex ternary operators. So if this would be changed for clarity (I think it's good as it currently is, it's the same pattern as the error above), then I'd rather say:
if (this.hasExchange()) {
this._bitswap.getMany(cids, callback)
} else {
asyncMap(cids, (cid, cb) => this._repo.blocks.get(cid, cb), callback)
}
There was a problem hiding this comment.
Got it. Yea I don't have strong feelings about ternary vs none-ternary either.
My concern was mainly that it looks like we're actually returning something synchronously, while the API is async.
There was a problem hiding this comment.
@PascalPrecht I agree that it is confusing to someone not used to that style. Perhaps early returns are more clear when written as:
if (this.hasExchange()) {
this._bitswap.getMany(cids, callback)
return
}
Though I don't have strong feelings about it. Either way is fine for me, but it should perhaps end up in the Coding Guideles to make it easy to get started.
daviddias
force-pushed
the
feat/getMany
branch
from
6367f15
to
48a3687
Compare
No description provided.