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

correctness fixes to get Envoy xDS protos compiling #30

Closed
wants to merge 7 commits into from

Conversation

bpowers
Copy link

@bpowers bpowers commented Jan 17, 2022

👋🏻 I'm working to read in some Envoy protobufs, and its a large project with a bunch of corner cases (e.g. fields named map, enum values named self and with the allow_alias option enabled, etc). The below commits are what I needed to get things at least compiling!

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I am so sorry for missing this and not reviewing this sooner, thank you for this contribution 👍

I left a minor nit, which you can take or leave, just let me know.

If you could please sign the CLA, I can get this in. It's pretty boilerplate, but let me know if there are any issues.

pbjson-build/src/escape.rs Outdated Show resolved Hide resolved
@@ -57,7 +58,7 @@ impl<'a> Resolver<'a> {
.path()
.iter()
.zip(path.path())
.filter(|(a, b)| a == b)
.take_while(|(a, b)| a == b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

.filter(|variant| {
// Skip duplicate enum values. Protobuf allows this when the
// 'allow_alias' option is set.
numbers.insert(variant.number())
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could get a test of this added to pbjson-test?

@kaizensparc
Copy link

Hi! Is there any news about this PR?

bpowers and others added 2 commits August 10, 2023 09:15
@sporkmonger
Copy link
Contributor

I've started working on it.

@tustvold
Copy link
Contributor

Closed by #103

@tustvold tustvold closed this Sep 17, 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.

4 participants