feat(bitswap.unwant) expose bitswap.unwant to cli and http api #1306
Conversation
ghost
assigned 
|
Tests were working when I checked this in now they're not, looking into why. |
wraithgar
force-pushed
the
bitswap_unwatch
branch
2 times, most recently
from
5417b46
to
ae478c6
Compare
wraithgar
changed the title
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
ae478c6
to
40e9e60
Compare
wraithgar
changed the title
|
Got an email from Travis saying tests passed on this. |
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
40e9e60
to
dc42483
Compare
src/cli/commands/bitswap/unwant.js
Outdated
| handler (argv) { | ||
| throw new Error('Not implemented yet') | ||
| argv.ipfs.bitswap.unwant(argv.key); |
There was a problem hiding this comment.
Should this also be wrapped in a try/catch?
There was a problem hiding this comment.
From the other cli files I looked at that use callbacks, if they get an error they just throw an error. I don't think the cli has a "catch and log to stderr" functionality in place.
I was using src/cli/commands/bitswap/wantlist.js and src/cli/commands/block/get.js as reference points on this because one is from the bitswap command list and the other is the "opposite" of unwant.
There was a problem hiding this comment.
Yes, you're totally right that we don't have a pattern for handling failures on CLI. That's probably issue-able, I'll add it to a todo. My thought was that we at least shouldn't print('Key.. removed') after a failure.
There was a problem hiding this comment.
As far as I can tell all failures will throw an exception so the print won't happen. Under the hood it's just removing an item from a set, once the CID validation passes (which would throw if there was an invalid key passed).
|
@wraithgar there seems to be a lint error that is failing your builds -
|
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
dc42483
to
5a46bdb
Compare
|
Thanks @dryajov. Fixed. Linting in this repo is quite a pain as there are currently 91 linting problems reported when I run |
|
Yeah, those warnings should definitely be taken care of at some point, but I believe they are very low priority ATM. Not sure if anyone has taken the time to investigate them tho. |
|
@wraithgar, thanks for submitting this PR. Yes, please update the spec -- https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/BITSWAP.md -- with docs about this method. |
|
The remaining tests in |
|
@wraithgar bitswap is the block exchange protocol, the block API calls are used to ensure that Bitswap is indeed doing its thing. Learn more about Bitswap at: |
|
This and the two PRs in interface and api are ready to go, as far as I can tell. |
wraithgar
force-pushed
the
bitswap_unwatch
branch
3 times, most recently
from
984d201
to
e309b36
Compare
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
e309b36
to
4ccac26
Compare
|
Please rebase master onto this branch for green CI |
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
4ccac26
to
81907be
Compare
|
This has been rebased w/ master. |
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
81907be
to
5993a9b
Compare
Will require including the version of interface-ipfs-core that they are moved to
wraithgar
force-pushed
the
bitswap_unwatch
branch
from
5993a9b
to
2ab7b02
Compare
|
closing as this was superseded by #1349 (now merged) |
I have a few questions about this PR.
I don't think thefigured this one out, it was how I was testing.src/core/components/bitswap.jsfile is being used during the tests (since they're online only they go through the http api)I can remove the print statement in this cli implementation and just test that no error happened instead if we want the output to match too.