-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add conversion to protobuf for most core types #514
Conversation
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.
Thanks for breaking this out - made it much easier to review.
b8ffa92
to
fdad6dd
Compare
Build Succeeded 🥳 Build Id: 8d18dc7a-1442-4428-af8a-10ec015c045b To build this version:
|
@@ -49,6 +50,22 @@ impl Default for Mode { | |||
} | |||
} | |||
|
|||
impl From<Mode> for ProtoMode { |
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.
Not sure if I was clear in the above comment which was resolved.
I was suggesting that we should follow the same pattern of using the proto
import alias as we have elsewhere in this PR for this file as well for consistency (and I like the pattern better).
So this should become:
impl From<Mode> for ProtoMode { | |
impl From<Mode> for proto::Mode { |
and so on and so forth for the other items in this file as well.
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.
The problem with doing it for these types, is that they're embedded within the compress
message namespace, so it would be proto::compress::Mode::Snappy
for example rather than proto::Mode::Snappy
. I could move the types outside the message in their Protobuf definition to fix that, but I wanted to make sure separately that that's what we want to do here separately.
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.
Got it - that makes sense. Thanks for the explanation. Approval from me then 👍🏻
This splits out most of the type conversion code from #506 in order to make it easier to review and merge.``