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

feat(docs): Add routeguide tutorial #21

Merged
merged 35 commits into from
Oct 18, 2019
Merged

feat(docs): Add routeguide tutorial #21

merged 35 commits into from
Oct 18, 2019

Conversation

alce
Copy link
Collaborator

@alce alce commented Oct 1, 2019

A work in progress tutorial for the routeguide example

@LucioFranco
Copy link
Member

This is looking really good! Let me know when you'd like a review pass and if you need anything! <3

@alce
Copy link
Collaborator Author

alce commented Oct 3, 2019

Thanks! I think it's ready for review up to the Creating the client section, which I'm working on now.

@alce
Copy link
Collaborator Author

alce commented Oct 5, 2019

@LucioFranco This is ready for the first review round.

@LucioFranco
Copy link
Member

@alce thank you so much for doing this. I am a bit short on time today so will try to get through this this week. Thanks <3

@LucioFranco LucioFranco self-requested a review October 8, 2019 15:06
@LucioFranco
Copy link
Member

cc @bIgBV

Copy link
Contributor

@bIgBV bIgBV left a comment

Choose a reason for hiding this comment

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

This is an awesome guide! I've left some comments, but they're mostly nits.

```shell
$ cargo run --bin routeguide-client
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a small line about what the user should expect to see at this step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bIgBV thank you for your comments.
I've added a small explanation of what the user should see

tonic-examples/routeguide-tutorial.md Outdated Show resolved Hide resolved
convenient because once we've set everything up, there is no extra step to keep the generated code
and our `.proto` definitions in sync.

Behind the scenes, Tonic uses [PROST!] to handle protocol buffer serialization and code
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the link text supposed to be capitalized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is how they write the project's name. It does look weird though.

let (mut tx, rx) = mpsc::channel(4);
let features = self.features.clone();

tokio::spawn(async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a great way to use the async-stream crate right? Maybe mention it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

async-stream is used on the bidirectional streaming example and we talk abut it briefly. Do you think we should use it here too or explain in a bit more detail what it does?

@LucioFranco
Copy link
Member

@alce just wanna check in, is this complete or is there more you wanted to add?

@alce
Copy link
Collaborator Author

alce commented Oct 18, 2019

I think this is ready for now. We'll need to tweak a couple of things for the next tonic release but it should be ready to go if you agree.

@LucioFranco LucioFranco changed the title [WIP] routeguide tutorial feat(docs): Add routeguide guide Oct 18, 2019
@LucioFranco LucioFranco changed the title feat(docs): Add routeguide guide feat(docs): Add routeguide tutorial Oct 18, 2019
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

@alce I just want to say thank you for bearing with me through all this and thank you so much for contributing this. Docs and tutorials is one of those things that differentiate projects so I really appreciate this contribution! <3

@LucioFranco LucioFranco merged commit 5d0a795 into hyperium:master Oct 18, 2019
blittable pushed a commit to blittable/tonic that referenced this pull request Oct 22, 2019
brentalanmiller pushed a commit to brentalanmiller/tonic that referenced this pull request Oct 6, 2023
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.

3 participants