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.

Sign up for GitHub

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

Jump to bottom

rely on impls more #556

Merged
merged 2 commits into from
Feb 27, 2025
Merged

rely on impls more #556

merged 2 commits into from
Feb 27, 2025

Conversation

cdisselkoen
Copy link
Contributor

  • Adds a generic instance for Field (Option a) to our generic Protobuf library, and uses it where applicable in CedarProto
  • Adds HAppend impls for Repeated and Packed types, and use that instead of Field.concatenate which only worked for Arrays (not Repeated or Packed)

@cdisselkoen
rely on impls more
ac1d0d7 
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
shaobo-he-aws
shaobo-he-aws approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws left a comment

Choose a reason for hiding this comment

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

LGTM

@cdisselkoen
updates from review
b85994b 
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
john-h-kastner-aws
john-h-kastner-aws approved these changes Feb 27, 2025
View reviewed changes
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

seems legit

@cdisselkoen cdisselkoen merged commit e60af35 into main Feb 27, 2025
6 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/impls branch February 27, 2025 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaobo-he-aws shaobo-he-aws shaobo-he-aws approved these changes

@john-h-kastner-aws john-h-kastner-aws john-h-kastner-aws approved these changes

@emina emina Awaiting requested review from emina

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cdisselkoen @shaobo-he-aws @john-h-kastner-aws