Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

Example refactoring #571

Merged
merged 45 commits into from
Aug 12, 2015
Merged

Example refactoring #571

merged 45 commits into from
Aug 12, 2015

Conversation

inetic
Copy link
Contributor

@inetic inetic commented Aug 11, 2015

To the new Routing API

Review on Reviewable

@maidsafe-highfive
Copy link

r? @benjaminbollen

(maidsafe_highfive has picked a reviewer for you, use r? to override)

use std::io;
use std::net::SocketAddr;
use std::str::FromStr;
use std::sync::{Arc, Mutex};
use std::sync::{Arc, Mutex, mpsc};

Choose a reason for hiding this comment

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

Can we now do without an Arc / Mutex ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can.

}
impl Client {
fn new(_bootstrap_peers: Vec<Endpoint>) -> Result<Client, RoutingError> {
let (routing_sender, routing_receiver) = mpsc::channel::<Event>();

Choose a reason for hiding this comment

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

just a style issue to consider: naming a channel WHAT_sender, and WHAT_receiver is more clear than WHO_sender/_receiver, as it feels to me less clear what the direction of the channel is

inetic and others added 2 commits August 11, 2015 15:42
Signed-off-by: Benjamin Bollen <benjamin.bollen@maidsafe.net>
@@ -20,9 +20,9 @@ use public_id::PublicId;
use types::Address;
Copy link
Member

Choose a reason for hiding this comment

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

We should rename the module / file here I think

Choose a reason for hiding this comment

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

I'll rename it in #570

@inetic inetic changed the title WIP: Example refactoring Example refactoring Aug 12, 2015
@benjaminbollen
Copy link

will already merge (in concert with @inetic s review of #570 - as both contain the others commits)
the libsodium issue needs resolving independent from this PR

benjaminbollen pushed a commit that referenced this pull request Aug 12, 2015
@benjaminbollen benjaminbollen merged commit 0aaf05b into maidsafe:master Aug 12, 2015
maqi added a commit to maqi/routing that referenced this pull request Nov 14, 2016
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.

None yet

4 participants