Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor FullMeshLinked and ConnectAll() #1617

Merged
merged 1 commit into from
Aug 28, 2015
Merged

Conversation

rht
Copy link
Contributor

@rht rht commented Aug 28, 2015

No description provided.

@jbenet jbenet added the status/in-progress In progress label Aug 28, 2015
@@ -273,10 +273,12 @@ func TestNetworkSetup(t *testing.T) {

func TestStreams(t *testing.T) {

mn, err := FullMeshConnected(context.Background(), 3)
mn, err := WithNPeers(context.Background(), 3)
Copy link
Member

Choose a reason for hiding this comment

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

I dont see what we lose by continuing to use FullMeshConnected in cases like these. Maybe just make FullMeshConnected be:

func FullMeshConnected(...) {
  mn, err := WithNPeers(context.Background(), 3)
  if err != nil {
    nil, err
  }
  mn.LinkAll()
  mn.ConnectAll()
  return mn, nil
}

i think this is different from @whyrusleeping's case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking of doing this one instead.

@rht rht force-pushed the rm-full-mesh-linked branch 3 times, most recently from 2af62d2 to ef0bf10 Compare August 28, 2015 09:10
@rht rht changed the title Mocknet: use explicit LinkAll() & ConnectAll() Refactor FullMeshLinked and FullMeshConnected Aug 28, 2015
@rht
Copy link
Contributor Author

rht commented Aug 28, 2015

Looks like there are 2 different ConnectAll() depending on how n1 == n2 is handled. Which one should be the default? The case where a node connects with itself is more common.

@jbenet
Copy link
Member

jbenet commented Aug 28, 2015

@rht i think some tests are counting on avoiding duplicates. maybe pass in a bool? or just have two functions? ConnectDistinctPairs() ?

@rht
Copy link
Contributor Author

rht commented Aug 28, 2015

ConnectAllButSelf()

@rht
Copy link
Contributor Author

rht commented Aug 28, 2015

Since ConnectAll() is never used directly (only from FullMeshConnected), it is not defined.

@jbenet
Copy link
Member

jbenet commented Aug 28, 2015

@rht LGTM

@rht rht changed the title Refactor FullMeshLinked and FullMeshConnected Refactor FullMeshLinked and ConnectAll() Aug 28, 2015
jbenet added a commit that referenced this pull request Aug 28, 2015
Refactor FullMeshLinked and ConnectAll()
@jbenet jbenet merged commit 61cde12 into ipfs:master Aug 28, 2015
@jbenet jbenet removed the status/in-progress In progress label Aug 28, 2015
@rht rht deleted the rm-full-mesh-linked branch August 30, 2015 01:21
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants