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

remove Loc from protobufs in AST.proto #1343

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Removes source locations from all the protobuf formats in AST.proto. They are not used by the Lean code which consumes them. And, since each Loc includes a full copy of the original source string, they blow up the encoded size of the protobufs significantly.

Breaking change, but protobufs is an experimental feature and this is exactly the kind of change folks should expect while the feature is experimental.

Issue #, if available

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

  • Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Craig Disselkoen <[email protected]>
Signed-off-by: Craig Disselkoen <[email protected]>
@cdisselkoen cdisselkoen removed the request for review from AMZ-brandon November 27, 2024 21:43
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.

Also needs the update in cedar-policy-cli

@cdisselkoen
Copy link
Contributor Author

Thanks for the reminder, done

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.

3 participants