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

Mock #87

Merged
merged 4 commits into from Jan 9, 2020
Merged

Mock #87

merged 4 commits into from Jan 9, 2020

Conversation

@madadam
Copy link
Contributor

madadam commented Jan 8, 2020

This PR adds mock version of quic-p2p as a workspace subcrate. The code itself is extracted from routing.

@madadam madadam requested a review from nbaksalyar Jan 8, 2020
@madadam madadam requested a review from maidsafe/backend_codeowners as a code owner Jan 8, 2020
@madadam madadam mentioned this pull request Jan 8, 2020

/// Events from `QuicP2p` to the user.
#[derive(Debug)]
pub enum Event {

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jan 8, 2020

Contributor

Since it is in the same repo, is it worth using the real event?

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Jan 8, 2020

Contributor

No, I guess since it does not have a dependency on quic_p2p, make sense.

This comment has been minimized.

Copy link
@madadam

madadam Jan 9, 2020

Author Contributor

Well, currently mock doesn't depend on the real thing, but maybe it should? Then we would be able to reuse some of the definitions. That could also make it a bit easier to ensure both APIs remain identical. What do you think?

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jan 9, 2020

Member

That could also make it a bit easier to ensure both APIs remain identical

This would make total sense, especially if we start to change the code to use async/await features. However, I think we can create an issue for now and tackle this later.

Copy link
Contributor

jeanphilippeD left a comment

Looks good to me assuming the routing part make sense too.

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor

jeanphilippeD commented Jan 9, 2020

Seems like you need to rust-fmt it and then I'll approve.

Copy link
Contributor

jeanphilippeD left a comment

To merge once fmt is fixed

Copy link
Member

nbaksalyar left a comment

Looks good to be merged after the rustfmt is fixed 👍

@jeanphilippeD

This comment has been minimized.

Copy link
Contributor

jeanphilippeD commented Jan 9, 2020

Branch looks out of date with master, seem it needs rebase?

So it can be used by other crates apart from routing.
@madadam madadam dismissed stale reviews from nbaksalyar and jeanphilippeD via 89dbc20 Jan 9, 2020
@madadam madadam force-pushed the madadam:mock branch from 3bc6b0a to 89dbc20 Jan 9, 2020
madadam added 3 commits Jan 9, 2020
@jeanphilippeD jeanphilippeD merged commit 5c023b7 into maidsafe:master Jan 9, 2020
5 checks passed
5 checks passed
Rustfmt-Clippy
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Test Publish
Details
@madadam madadam deleted the madadam:mock branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.