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

Add conversion to protobuf for most core types #514

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Conversation

XAMPPRocky
Copy link
Collaborator

This splits out most of the type conversion code from #506 in order to make it easier to review and merge.``

Copy link
Contributor

@markmandel markmandel left a 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.

src/filters/load_balancer.rs Show resolved Hide resolved
src/filters/compress/config.rs Show resolved Hide resolved
@XAMPPRocky XAMPPRocky requested a review from markmandel April 13, 2022 08:19
@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 8d18dc7a-1442-4428-af8a-10ec015c045b

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/514/head:pr_514 && git checkout pr_514
cargo build

@@ -49,6 +50,22 @@ impl Default for Mode {
}
}

impl From<Mode> for ProtoMode {
Copy link
Contributor

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:

Suggested change
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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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 👍🏻

@markmandel markmandel merged commit 3952a71 into main Apr 13, 2022
@markmandel markmandel deleted the ep/conversion-impl branch April 13, 2022 19:49
@markmandel markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Refactoring code, fixing up documentation, etc size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants