-
Notifications
You must be signed in to change notification settings - Fork 5
chore: Preparation for publishing SDK #31
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
Conversation
src/grpc/cache_header_interceptor.rs
Outdated
let mut result = tonic::Request::new(request.into_inner()); | ||
let mut result = { | ||
request.into_inner(); | ||
tonic::Request::new(()) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing to me. Can you explain what's happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I need to update that actually, but that structure is suggested by clippy.
It gave me this error: error: passing a unit value to a function
So I think what I want to do here:
let mut result = {
let message = request.into_inner();
tonic::Request::new(message)
};
Reference: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if request.into_inner() returns unit then why are we calling it? Or does it not return unit and this was actually a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm it does not look like request.into_inner()
is returning unit?
Clippy might have given us false positive? rust-lang/rust-clippy#1502
https://docs.rs/tonic/latest/tonic/struct.Request.html#method.into_inner
/// Consumes `self`, returning the message
pub fn into_inner(self) -> T {
self.message
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
into_inner()
returns T
, as your snippet shows. request
is tonic::Request<()>
. T
is ()
. So why are we calling .into_inner()
on request
?
/// Momento Service encountered an unexpected exception while trying to fulfill the request. | ||
InternalServerError(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these generated or did you add them?
If you added them, do ///
formatted comments show up in typical ides like IntelliJ and VS Code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them and these are for rust documentation that will appear in our package's doc website when we publish it to crates.io.
https://doc.rust-lang.org/rust-by-example/meta/doc.html
For example, mio has this doc website and anything in ///
appear in that doc.
Source code: https://docs.rs/mio/0.8.2/src/mio/interest.rs.html#4-15
Mio's doc website:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I prefer ///
docs anyway, thanks for confirming this is by design.
tonic::metadata::AsciiMetadataValue::from_str(&cache_name).unwrap(), | ||
tonic::metadata::AsciiMetadataValue::from_str(cache_name).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels good cleaning all this stuff up huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YUP! Thank you Clippy.
README.md
Cargo.toml
Ticket: #20