Skip to content
This repository was archived by the owner on Aug 11, 2021. It is now read-only.

[for consideration] Move the notion of a 'online' block service to 'hasExchange' #61

Closed
daviddias opened this issue Mar 21, 2017 · 8 comments

Comments

@daviddias
Copy link
Member

daviddias commented Mar 21, 2017

ref:

Once we start having more exchanges, it will make it more obvious why this is interesting.

@daviddias daviddias changed the title [for consideration] Move the notion of a 'online' block service to 'hasExchange [for consideration] Move the notion of a 'online' block service to 'hasExchange' Mar 21, 2017
@daviddias
Copy link
Member Author

//cc @dignifiedquire

@dignifiedquire
Copy link
Member

I forgot to respond in the PR sorry. I generally like the idea, the online and offline names are pretty confusing

@daviddias daviddias added the status/ready Ready to be worked label Apr 5, 2017
@daviddias daviddias added status/deferred Conscious decision to pause or backlog and removed status/ready Ready to be worked labels Jun 20, 2017
@pgte
Copy link
Contributor

pgte commented Jul 1, 2017

Simply renaming .goOnline to .addExchange is not accurate, as it would overwrite the previous bitswap. Should it be setExchange?

@daviddias
Copy link
Member Author

@pgte agreed. What about the 'disable', rmExchange?

@pgte
Copy link
Contributor

pgte commented Jul 2, 2017

That, or unsetExchange. Perhaps consider also acting on setExchange(null).

@dignifiedquire
Copy link
Member

If the goal is to support multiple exchanges I wonder if it wouldn't be better to expose an api like this

.exchanges.ls()
// => returns an array of existing exchanges
.exchanges.add(newExchange)
// => adds a new exchange to the list
.exchanges.rm(existingExchange)
// => removes the given exchange (might want to this by name or id or something like that 
.exchanges.clear()
// => removes all exchanges
.exchanges.has()
// => alias for .ls().length > 0

@daviddias
Copy link
Member Author

I'm good either way. I agree that if we get more than one exchange it will be good to have such an API, but at the same time, we don't have any other nor plans for it.

Right now what is important to get fixed is the misleading method name.

pgte added a commit that referenced this issue Jul 3, 2017
@daviddias
Copy link
Member Author

Done with #67

@daviddias daviddias removed the status/deferred Conscious decision to pause or backlog label Jul 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants