Skip to content

Add Rust Getting Started Codelab #51

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

cjqzhao
Copy link
Collaborator

@cjqzhao cjqzhao commented Jul 24, 2025

Add Getting Started Rust Codelab

@easwars @dfawley

Choose a reason for hiding this comment

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

Usually the .proto files are placed inside a /proto directory at the same level as /src. See tonic's health crate as an example: https://github.com/hyperium/tonic/tree/master/tonic-health

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok done!


message Point {
/////////////////////////////////////////////////////////////////////////////
// Codelab Hint: Define RPC methods here

Choose a reason for hiding this comment

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

The comment needs to say message fields instead of "RPC methods". Same for the Feature message below.

```sh
$ cd tonic/protoc-gen-rust-grpc
$ bazel build //src:protoc-gen-rust-grpc
$ PLUGIN_PATH="$(pwd)/bazel-bin/src/protoc-gen-rust-grpc"

Choose a reason for hiding this comment

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

nit: We don't need to set PLUGIN_PATH, it's not used later.

Choose a reason for hiding this comment

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

This should not be empty, it causes cargo build to fail.

Comment on lines 42 to 43


Choose a reason for hiding this comment

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

nit: Please remove extra blank lines here and at the end of the file.

// env!("CARGO_MANIFEST_DIR"),
// "/generated/generated.rs"
// ));
// include!(concat!(

Choose a reason for hiding this comment

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

nit: If there's a comment for including the message code, we should also have a comment for including the service code.

// .include("src/routeguide")
// .inputs(["routeguide.proto"])
// .output_dir("generated")
// .compile_only()

Choose a reason for hiding this comment

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

nit: The compile_only option isn't present for the service codegen.

.unwrap();

// NOTE: If you were to generate code yourself, you would use the
// command below using tonic_protobuf_build's CodeGen.

Choose a reason for hiding this comment

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

nit: I think we should also mention that the above command should to be removed.

async fn get_feature(&self, request: Request<Point>) -> Result<Response<Feature>, Status> {
println!("GetFeature = {:?}", request);
let requested_point = request.get_ref();
for feature in &self.features[..] {

Choose a reason for hiding this comment

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

nit: for feature in self.features.iter() seems less verbose.

Choose a reason for hiding this comment

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

In my opinion, it will be simpler to just combine this with server.rs and have the load function at the end of the file, below main.

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