Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

Helper to get PeerInfo from Host #20

Merged
merged 1 commit into from
Oct 24, 2018
Merged

Helper to get PeerInfo from Host #20

merged 1 commit into from
Oct 24, 2018

Conversation

raulk
Copy link
Member

@raulk raulk commented Oct 24, 2018

Trivial change to make this easier based on user feedback.

go-libp2p-host already imports the go-libp2p-peerstore, so no strings attached.

@ghost ghost assigned raulk Oct 24, 2018
@ghost ghost added the in progress label Oct 24, 2018
@raulk raulk requested a review from bigs October 24, 2018 16:26
@Stebalien
Copy link
Member

This should probably get a test.

@raulk
Copy link
Member Author

raulk commented Oct 24, 2018

@Stebalien didn't think it would add much value, since the function just creates a struct, but I pushed it in 1edd98c. Couldn't think of a way other than creating a mock host, or importing another module (which we don't want to do). LGTY?

@raulk
Copy link
Member Author

raulk commented Oct 24, 2018

Alternatively we could have an interface that's a partial view on Host with only ID() and Addrs(), but I really don't want to do that as it also reduces discoverability by the libp2p newbie.

@Stebalien
Copy link
Member

Oh, nevermind. I'm just working on a knee-jerk needs-test reaction. The test is more likely to break than the code.

@raulk
Copy link
Member Author

raulk commented Oct 24, 2018

@Stebalien I feel you; I'll rm this test.

@raulk raulk merged commit e758e0c into libp2p:master Oct 24, 2018
@raulk raulk deleted the feat/helper branch October 24, 2018 17:22
@ghost ghost removed the in progress label Oct 24, 2018
@lthibault
Copy link

lthibault commented Oct 24, 2018

Why does this new function return a *PeerInfo when methods like Host.Connect take a concrete PeerInfo?

The API seems quite inconsistent, here. Am I missing something?

@raulk
Copy link
Member Author

raulk commented Oct 25, 2018

@lthibault for consistency with pstore.InfoFromP2pAddr() and pstore.InfoToP2pAddrs(), both of which use pointers.

We do return a concrete PeerInfo, just allocated in the heap.

@lthibault
Copy link

@raulk

We do return a concrete PeerInfo, just allocated in the heap.

Are you using the term "concrete" to mean "not an interface"? If so, this is just semantics :)

More to the point, shouldn't Host.Connect() take a pointer, then? Again, this makes the API feel quite inconsistent from the outside.

@raulk
Copy link
Member Author

raulk commented Oct 25, 2018

@lthibault it could, but i don't really see the value. Pointers don't make an API feel inconsistent; they're used for technical reasons. The actual types are the same.

That said, these structs are small enough that copy-on-return wouldn't hurt. But this change is not worth the breakage.

@Stebalien
Copy link
Member

Yeah, in most places we try to return these by value but it's really not that much of an issue (and I agree with being consistent).

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