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.

Sign up for GitHub

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

Jump to bottom

Tidy up bootstrapping #225

Merged
merged 7 commits into from
Jan 24, 2019
Merged

Tidy up bootstrapping #225

merged 7 commits into from
Jan 24, 2019

Conversation

anacrolix
Copy link
Contributor

Refactor bootstrapping to remove goprocess, and use contexts properly. Also includes some interface assertions for future refactoring.

@anacrolix anacrolix requested a review from bigs January 23, 2019 05:33
@ghost ghost assigned anacrolix Jan 23, 2019
@ghost ghost added the status/in-progress In progress label Jan 23, 2019
vyzo
vyzo reviewed Jan 23, 2019
View reviewed changes
routing.go Show resolved Hide resolved
raulk
raulk reviewed Jan 23, 2019
View reviewed changes
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Initial feedback written from mobile.

dht_bootstrap.go Show resolved Hide resolved
dht.go Show resolved Hide resolved
dht_bootstrap.go Outdated Show resolved Hide resolved
dht_bootstrap.go Show resolved Hide resolved
routing.go Show resolved Hide resolved
raulk
raulk requested changes Jan 23, 2019
View reviewed changes
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Replied to comments; sorry for blocking, but some items need addressing.

@anacrolix
Copy link
Contributor Author

The PR now also includes the removal of BootstrapOnSignal with a replacement, and its demonstration in the tests.

anacrolix
anacrolix commented Jan 23, 2019
View reviewed changes
dht_test.go Show resolved Hide resolved
@anacrolix anacrolix mentioned this pull request Jan 23, 2019
Merged
@Stebalien
Copy link
Member

We do not break things without good reason. We have many users simply using go get and we try very hard to not put unnecessary burden on our users by changing things at whim.

@hsanjuan
Copy link
Contributor

BootstrapOnce is an acceptable replacement for BootstrapOnSignal for me. As I'm the only known user for that I'm OK changing that this way.

@Stebalien
Copy link
Member

We do not break things without good reason. We have many users simply using go get and we try very hard to not put unnecessary burden on our users by changing things at whim.

WRT this patch, I actually agree with adding the context argument to those functions. I just want to avoid using "downstream users can deal" as an excuse.

@anacrolix anacrolix merged commit 66cc80c into master Jan 24, 2019
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@raulk
Copy link
Member

raulk commented Jan 24, 2019

Was this pull request ever approved before merging?

@Stebalien
Copy link
Member

No.

@Stebalien Stebalien mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsanjuan hsanjuan hsanjuan left review comments

@vyzo vyzo vyzo left review comments

@raulk raulk raulk requested changes

@bigs bigs Awaiting requested review from bigs

Assignees

@anacrolix anacrolix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anacrolix @Stebalien @hsanjuan @raulk @vyzo