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

feat: bitswap.wantlist peerid #1349

Merged
merged 10 commits into from
Jun 20, 2018
Merged

feat: bitswap.wantlist peerid #1349

merged 10 commits into from
Jun 20, 2018

Conversation

wraithgar
Copy link
Contributor

@wraithgar wraithgar commented May 10, 2018

Because of the amount of work done in #1306 this PR is built off of that, that PR needs to be merged before this one can be isolated and discussed.

This adds a peerId parameter to bitswap.unwant.

Tests are in ipfs-inactive/interface-js-ipfs-core#267
API changes are in ipfs-inactive/js-ipfs-http-client#761

@ghost ghost assigned wraithgar May 10, 2018
@ghost ghost added the status/in-progress In progress label May 10, 2018
})
})
})
})
Copy link
Member

@daviddias daviddias May 12, 2018

Choose a reason for hiding this comment

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

note to self and other readers, tests were moved to ipfs-inactive/interface-js-ipfs-core#267

@daviddias
Copy link
Member

Please rebase master onto this branch for green CI

@wraithgar
Copy link
Contributor Author

This has been rebased w/ the version of #1306 that was rebased against master.

Tests probably won't pass without the changes to js-ipfs-api in place

},
let list
if (peerId) {
peerId = PeerId.createFromB58String(peerId)
Copy link
Member

Choose a reason for hiding this comment

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

A try/catch around this will allow us to callback with an error for invalid user input.

process.stdout.write(block.data)
} else {
process.stderr.write('Block was unwanted before it could be remotely retrieved')
}
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd use print from utils so that output can be disabled (I'm not sure why you'd want to do that for this call but we should probably support it!).

}

// TODO: implement when https://github.com/ipfs/js-ipfs-bitswap/pull/10 is merged
}
callback(null, self._bitswap.unwant(key))
Copy link
Member

Choose a reason for hiding this comment

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

setImmediate here too?

list = self._bitswap.getWantlist()
}
list = formatWantlist(list)
callback(null, list)
Copy link
Member

Choose a reason for hiding this comment

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

setImmediate here?

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

I just noticed there's missing tests for the bitswap CLI, we need a test for wantlist with a peer and a test for unwant.

Could you also enable testing of the http interface with interface-ipfs-core by dropping a bitswap.js file in here.

Awesome work BTW 🚀

@daviddias daviddias changed the title Bitswap wantlist peerid feat: bitswap.wantlist peerid May 29, 2018
@alanshaw alanshaw mentioned this pull request Jun 1, 2018
23 tasks
@alanshaw
Copy link
Member

alanshaw commented Jun 8, 2018

@wraithgar whats the status on this? Can I do to help you pull this over the line?

@wraithgar
Copy link
Contributor Author

@alanshaw have this rebased against the PRs surounding #1306 and am updating it to make tests pass again. This should address some of your comments above.

@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch 2 times, most recently from 5953a59 to 371bb56 Compare June 8, 2018 22:57
@wraithgar
Copy link
Contributor Author

Ok the rebase and test updates are done.

Updated original comment to link to API PR.

Still needs the CLI tests but everything else seems to be in place and working with both go (via the api tests) and js (via these tests)

@wraithgar
Copy link
Contributor Author

CLI tests added and bugs found therein fixed.

@alanshaw
Copy link
Member

@wraithgar there seems to be linting and build issues still with this - would you mind taking a look?

@wraithgar wraithgar force-pushed the bitswap_wantlist_peerid branch 2 times, most recently from afc9ba3 to 2733a12 Compare June 14, 2018 15:22
@wraithgar
Copy link
Contributor Author

@alanshaw Linting errors fixed. Build errors are because this depends on the PRs in both the API and Interface.

@alanshaw
Copy link
Member

@wraithgar FYI for the future, to appease the commit linter your commit message prefixes need a colon after them

Will require including the version of interface-ipfs-core that
they are moved to
Will require including the version of interface-ipfs-core that
they are moved to
wraithgar and others added 3 commits June 18, 2018 17:45
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
wraithgar and others added 3 commits June 19, 2018 09:52
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@alanshaw
Copy link
Member

It green!

@alanshaw alanshaw merged commit 45b705d into master Jun 20, 2018
@ghost ghost removed the status/in-progress In progress label Jun 20, 2018
@alanshaw alanshaw deleted the bitswap_wantlist_peerid branch June 20, 2018 06:28
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.

3 participants