Grant/issue10 #55
Grant/issue10 #55
Conversation
|
|
||
| const result = await bchjs.Ninsight.details(addr) | ||
| //console.log(`result: ${JSON.stringify(result, null, 2)}`) | ||
| assert.property(result[0], "balance") |
There was a problem hiding this comment.
check first if it is Array assert.isArray(result)
test/unit/ninsight.js
Outdated
| } | ||
| }) | ||
|
|
||
| it(`should GET details for a single address`, async () => { |
test/unit/ninsight.js
Outdated
| assert.property(result, "pagesTotal") | ||
| }) | ||
|
|
||
| it(`should GET details for an array of addresses`, async () => { |
There was a problem hiding this comment.
should POST - it is getting details via POST method
test/unit/ninsight.js
Outdated
| it(`should GET details for a single address`, async () => { | ||
| // Stub the network call. | ||
| sandbox.stub(axios, "post").resolves({ | ||
| data: mockData.addrDetail |
There was a problem hiding this comment.
This is strange. According to your integration tests both single address and multiply addresses returns Array (cause they are both using POST, this is different from original BitBox call), so should be addrDetailArray here...
There was a problem hiding this comment.
OK, I also realized that and then I thought it could be a feature not a bug :)? maybe the API is easier to use if the return type is always the same? What do you think? Would it make sense to change the API in that direction? I can also just implement it differently, but that would also add more code complexity. I think on both sides of the interface varying return types would add complexity. Please tell me what to do! I am just brainstorming here.
There was a problem hiding this comment.
bch-js is just a JS wrapper for accessing to bch-api. API is already implemented and already running, so pretty much you cannot decide by yourself should it be GET or POST and should it be array or no. In this case API is POST requests for both single and multiply addresses, returning Array. So that should be bch-js implementing.
There was a problem hiding this comment.
A, also maybe you need to rebase from master again - "This branch is out-of-date with the base branch" message below
There was a problem hiding this comment.
I changed to mock addrDetailArray Is it ok now?
In the issue description there is something written about POST:
Porting the code should meet the following requirements:
The code should follow the same pattern as the existing utxo() method, but should only use the POST call to the REST API. This should include api-doc documentation.
There was a problem hiding this comment.
Seems good to me. But I'm just another junior developer. Chris will have the last word as a senior developer and product owner.
There was a problem hiding this comment.
@zh thanks for taking the time to review @christroutner what do you think, can we put a check mark on this one?
|
@zh POST, isArray check, 100% test |
closes #10