100% (line) coverage.

[hackergrrl]

Woohoo! This PR contains both important bugfixes and also test cases that prove their newfound correctness!

Important: this also changes the getBlock() callback semantics to NOT populate err when a block cannot be found. It is this hacker's belief that "not found" is not an error case. The README is also updated to reflect this!

This gets us to 100% line coverage. There are a few tiny branches still not covered, but they are both a) trivial, and b) would require test case bloat / noise to cover them.

Addresses ipfs/js-ipfs#115.

[daviddias]

Thank you @noffle :):)

I'm not sure about the hacker belief you describe. AFAIK, ENOENT is a thing and I've checked the fs module and abstract-blob-store and both of them yield a error if a thing is not found locally.

fs

> fs.readFile('__dirname' + '/random-file', {}, (err, data) => {if (err) {console.log('->', err)}})
undefined
> -> { [Error: ENOENT: no such file or directory, open '__dirname/random-file']
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '__dirname/random-file' }

abstract-blob-store

https://github.com/maxogden/abstract-blob-store/blob/master/tests/read-write.js#L52-L65

[hackergrrl]

It's true: we could check Error.code for ENOENT, but this is an awkward way to check for non-existence (it IS an error). I'd feel better about it if we had an API call for checking block existence as well -- much like how blob-stores have exists().

This raises the question of how to handle partial failure in getBlocks. One way is to accumulate an array of errors to return alongside the array of blocks.

[hackergrrl]

I poked at some folks on #stackvm and thought some more about this, and I agree that getBlock returning an error here is reasonable: if the user is calling getBlock then they are making an implicit assertion that it already exists. If it doesn't, an error is a reasonable outcome to codify this contradictory state.

Now to decide how getBlocks (plural) should behave. It could simply swallow ENOENTs and return whatever blocks come back, or it could fail noisily the instant one of the requested blocks is not present.

[hackergrrl]

Can we config the coveralls bot to not post on the issue thread? It's noisy and the coverage notes are already included in the merge results box at the bottom.

[whyrusleeping]

agreed. its super noisy.

[daviddias]

Agreed. Removed the coveralls comments (if you happen to see another one in another repo, I might have missed).

@noffle in relation to getBlocks, what we can do is to return an array of objects with {multihash:<>, block: <>} and in the ones that it was not possible to retrieve, we set it to undefined, so that in a IPFS core get several blocks call, they can be first retrieved by the repo and those who are missing can be populated by bitswap or other mechanism.

[hackergrrl]

@diasdavid it seems strange if getBlocks doesn't return an error state on not-found, but getBlock does. I'd really want us to be consistent across these APIs.

[daviddias]

Ok, let's put the {multihash: <>, err: <>} for the ones that fail to fetch from the repo

[hackergrrl]

Updated impl, tests, and docs. This is a breaking API change, so please bump the major after PRing. :)

[coveralls]

Coverage increased (+20.8%) to 100.0% when pulling 58f1191 on noffle:coverage into a907f86 on ipfs:master.

[whyrusleeping]

@coveralls your presence is unwelcome here

[hackergrrl]

@coveralls unsubscribe

[daviddias]

Disabled coveralls commenting (missed this one, sorry).

On bumping up major version. AFAIK and what I've been considering best practice is that until 1.0.0, the module should be handled as alpha/beta, so what I would do in this scenario is bumping minor. If we were already at major versions (after having declared that we had something stable) I would bump the major. Sounds good? Thoughts? @dignifiedquire you might have a opinion on this too.

[daviddias]

nice :)

[dignifiedquire]

:D :D I never have opinions ;)
But if you ask I'll give you some, I usually follow the convention node.js used before 1.0 if there is no 1.0 yet, meaning

• minor releases can mean breaking changes
• patch releases can include features and bug fixes, but should be backwards compatible.

[daviddias]

thank you @dignifiedquire :)

@noffle would you agree with that approach? :)

[hackergrrl]

Whoops, I didn't realize we were pre-1.0.0. Yeah -- bump whatever freely. 😀

[daviddias]

awesome! :D