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(examples): distributed-key-value-store identify and rendezvous #5571

Closed
wants to merge 8 commits into from

Conversation

P1R
Copy link
Contributor

@P1R P1R commented Aug 27, 2024

Description

Following on issue #4449
Refactor and cleanup on three examples:
refactor: use tokio instead of async-std in the distributed-key-value-store example and remove unnecesary dependencies
refactor: remove unnecesary dependencies rendezvous example (async-std)
refactor: use tokio instead of async-std in the identify example and remove unnecesary dependencies

Notes & open questions

  • in the distributed-key-value example is it ok to put "EOL"in the expect?
  • in the rendezvous example there where unnecessary dependencies in the Cargo.toml (so as in the identify)

Change checklist

  • Removed unecesary dependencies on examples/rendezvous/Cargo.toml and examples/identify/Cargo.toml

  • Changed async-std for tokio in examples/distributed-key-value-store/Cargo.toml and examples/identify/Cargo.toml

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@guillaumemichel guillaumemichel changed the title Refactor: Examples distributed-key-value-store, identify and rendezvous (Code Review) refactor(examples): distributed-key-value-store, identify and rendezvous (Code Review) Aug 27, 2024
@guillaumemichel guillaumemichel changed the title refactor(examples): distributed-key-value-store, identify and rendezvous (Code Review) refactor examples: distributed-key-value-store, identify and rendezvous Aug 27, 2024
@guillaumemichel guillaumemichel changed the title refactor examples: distributed-key-value-store, identify and rendezvous refactor(examples): distributed-key-value-store identify and rendezvous Aug 27, 2024
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi David, and thanks for this! Overall looks good to me, can we separate this PR into one for each example? Thanks!

@@ -72,15 +72,16 @@ async fn main() -> Result<(), Box<dyn Error>> {
swarm.behaviour_mut().kademlia.set_mode(Some(Mode::Server));

// Read full lines from stdin
let mut stdin = io::BufReader::new(io::stdin()).lines().fuse();
Copy link
Member

@jxs jxs Aug 28, 2024

Choose a reason for hiding this comment

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

can we keep std::futures::select! and therefore this fuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jxs i did as just line following other example implementations with tokios as the following:
chat/src/main.rs:88: let mut stdin = io::BufReader::new(io::stdin()).lines();
ipfs-private/src/main.rs:172: let mut stdin = io::BufReader::new(io::stdin()).lines();

but I can try your approach later tonight.

@P1R
Copy link
Contributor Author

P1R commented Aug 28, 2024

Sure, I will rewrite pull request for each example I forgot to ask that but I had it in mind. Thanks.

@P1R P1R changed the title refactor(examples): distributed-key-value-store identify and rendezvous refactor(examples): identify Aug 29, 2024
@P1R P1R closed this Aug 29, 2024
@P1R P1R changed the title refactor(examples): identify refactor(examples): distributed-key-value-store identify and rendezvous Aug 29, 2024
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