Rename IpfsDHT to Node #223
Conversation
ghost
assigned 
| // It is used to implement the base IpfsRouting module. | ||
| type IpfsDHT struct { | ||
| type Node struct { |
There was a problem hiding this comment.
If we're keeping this struct public, I'd rather prefix it with DHT, and remove the DHT from the functions below.
But actually I wonder if there's any use case to keep Node public; I suspect no. In that case I'd be happy with an unqualified node type demoted to private scope.
There was a problem hiding this comment.
the interface is wider than the routing interface; we would need a new interface for the DHT.
There was a problem hiding this comment.
To my knowledge, returning a private type allows consumers to access all its public methods, so you don't need to access them through an interface. In this case, the benefit is that it disallows constructing a struct by itself, and it doesn't leak the unqualified Node type to the public namespace.
BTW – I generally agree that there may be more interfaces to extract.
There was a problem hiding this comment.
You can't construct a variable of it's type though, so you can't assign it anywhere which makes it borderline useless.
Note that the interface of the DHT is wider than any extant interface we have.
There was a problem hiding this comment.
Right, you can assign it to a local variable with the := operator, but to pass it around in function arguments, you need to declare those args as interfaces. I'd argue that's the correct way, though.
There was a problem hiding this comment.
I think it should be a public type, and that type should be returned explicitly by its constructors. See the Go idiom on returning concrete types and accepting interfaces.
| @@ -95,7 +97,7 @@ func New(ctx context.Context, h host.Host, options ...opts.Option) (*IpfsDHT, er | |||
| // NewDHT creates a new DHT object with the given peer as the 'local' host. | |||
| // IpfsDHT's initialized with this function will respond to DHT requests, | |||
| // whereas IpfsDHT's initialized with NewDHTClient will not. | |||
| func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *Node { | |||
There was a problem hiding this comment.
How about we take the plunge and rename this func to NewNode? It's more idiomatic in Go, as the package is already named dht:
node := dht.NewNode()
We'd need to introduce a method alias, though.
There was a problem hiding this comment.
Yes, I concur. I also want to rename NewDHTClient to NewClientNode, or NewPassiveNode.
There was a problem hiding this comment.
let's not unnecessarily break existing code.
There was a problem hiding this comment.
An alias would be added for a period for backwards compatibility until the next major version.
| @@ -107,15 +109,15 @@ func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| // host. IpfsDHT clients initialized with this function will not respond to DHT | |||
| // requests. If you need a peer to respond to DHT requests, use NewDHT instead. | |||
| // NewDHTClient creates a new DHT object with the given peer as the 'local' host | |||
| func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |||
| func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching) *Node { | |||
There was a problem hiding this comment.
As per the above comment, this could become:
func NewClient(ctx context.Context, h host.Host, dstore ds.Batching) *node {
There was a problem hiding this comment.
Note that while you can return private types and still operate on them, they're concealed from docs and a bit dubious. Unfortunately being able to arbitrarily instantiate any public type without going through a constructor is just a Go wart, and not worth trying to evade.
|
I think we could retain IpfsDHT for backwards compatibility with the various interfaces people expect of the DHT, and break out Node as the actual DHT implementation. I'll close this for now, as it's easy enough to regenerate in the future when we have more information. |
Renames
IpfsDHTtoNodeper #191 (comment), and adds an alias for compatibility. It doesn't rename the receivers (which are alldht, and probably benode), or the constructors (NewDHTand friends). Those are easy enough to do subsequently if the PR is accepted. Tested againstgo-libp2p-daemon, which is the mainlibp2pconsumer of this package at present.