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 pbjson feature for Protobuf JSON serde #6

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

mbrobbel
Copy link
Member

@mbrobbel mbrobbel commented Sep 20, 2022

This adds an optional pbjson feature that uses pbjson to add Serialize and Deserialize implementations based on the Protobuf JSON mapping.

I've also refactored the build.rs file to use the walkdir crate (instead of glob), and use std::path types instead of strings.

This builds on #5 and relies on influxdata/pbjson#67, so marking this as draft for now.

@mbrobbel mbrobbel marked this pull request as ready for review September 21, 2022 11:47
@mbrobbel mbrobbel marked this pull request as draft September 21, 2022 14:18
@mbrobbel mbrobbel marked this pull request as ready for review September 28, 2022 14:56
Copy link

@jvanstraten jvanstraten left a comment

Choose a reason for hiding this comment

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

I don't see anything weird with this, nor any reason not to include pbjson support. @andygrove?

@CLAassistant
Copy link

CLAassistant commented Oct 6, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Apologies for the late review. LGTM.

@jvanstraten jvanstraten merged commit 43f97ef into substrait-io:main Nov 9, 2022
@mbrobbel mbrobbel deleted the pbjson branch November 9, 2022 08:59
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