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

Removes __expr #333

Merged
merged 28 commits into from
Oct 27, 2023
Merged

Removes __expr #333

merged 28 commits into from
Oct 27, 2023

Conversation

aaronjeline
Copy link
Contributor

Description of changes

__expr was deprecated. Now it is removed

Issue #, if available

N/A

Checklist for requesting a review

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

  • A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).

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

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

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

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

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aaronjeline
Copy link
Contributor Author

This does leave __expr as a reserved keyword, I wasn't sure if we wanted to remove that?

@cdisselkoen
Copy link
Contributor

I'm in favor of keeping it reserved even if only to provide better error messages for folks using it. (Speaking of, does this PR keep around tests that use __expr to ensure we throw errors, and sane error messages? Note that I haven't reviewed before writing this comment)

@aaronjeline
Copy link
Contributor Author

cedar-java is broken by this change, issue for updating it: cedar-policy/cedar-java#51

@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from 08abff9 to af7e68b Compare October 4, 2023 14:55
@aaronjeline aaronjeline requested a review from khieta October 5, 2023 20:02
@andrewmwells-amazon andrewmwells-amazon mentioned this pull request Oct 5, 2023
Merged
cdisselkoen
cdisselkoen approved these changes Oct 6, 2023
View reviewed changes
cedar-policy-cli/tests/integration_tests/main.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities.rs Show resolved Hide resolved
cedar-policy-core/src/entities/json/jsonvalue.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/jsonvalue.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/jsonvalue.rs Outdated Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from 26690f3 to 29a0e25 Compare October 9, 2023 17:18
@aaronjeline
Copy link
Contributor Author

Semver check fails as expected

cdisselkoen
cdisselkoen approved these changes Oct 12, 2023
View reviewed changes
cedar-policy-core/src/entities/json/err.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/jsonvalue.rs Outdated Show resolved Hide resolved
@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from 39495e9 to a4672ce Compare October 12, 2023 18:21
@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from a4672ce to 88a0fb1 Compare October 24, 2023 19:41
cdisselkoen
cdisselkoen reviewed Oct 25, 2023
View reviewed changes
cedar-policy-core/src/ast/request.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/err.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/value.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/value.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/value.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/value.rs Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
panic_safety.sh
panic_markers=("panic unwrap_used expect_used fallible_impl_from unreachable indexing_slicing")
panic_markers=("unwrap_used expect_used fallible_impl_from unreachable indexing_slicing panic")
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why this?

@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from c844e01 to b836970 Compare October 26, 2023 18:15
aaronjeline and others added 15 commits October 27, 2023 15:59
@aaronjeline
Passing on all non-corpus tests
7c9154f
@aaronjeline
Integrations tests in cli pass
a919b5d
@aaronjeline
Updating some tests
6ccb1be
@aaronjeline
Adding panic safety annotations
a7ee61a
@aaronjeline
Updated changelog
65ab553
@aaronjeline
Updated docs to remove __expr
7209341
@aaronjeline
Updating a test
7a0b19b
@aaronjeline
Updating tests
e97a511
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/jsonvalue.rs
715efb3 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities.rs
e423eb4 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/jsonvalue.rs
0f18131 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/jsonvalue.rs
c265a8a 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline
Updating panic safety
b558dbb
@aaronjeline
Keeping result a result
f52e09a
@aaronjeline
Ensure __expr is an error
f46343f
aaronjeline and others added 13 commits October 27, 2023 16:00
@aaronjeline
Fixing some mis-merges
95f178a
@aaronjeline
Fixing changelog duplicates
d8aa9ce
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/err.rs
ad8d863 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/jsonvalue.rs
ee422b7 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline
Improving error messages
82e735b
@aaronjeline
Bringing up to mainline
b7cd009
@aaronjeline
Fixing some tests
d890c62
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/ast/request.rs
6c8151e 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/value.rs
d5644f5 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/value.rs
b94f1b3 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline @cdisselkoen
Update cedar-policy-core/src/entities/json/value.rs
6d73595 
Co-authored-by: Craig Disselkoen <[email protected]>
@aaronjeline
Responding to comments
fbbb137
@aaronjeline
Added missing docs
08820de
@aaronjeline aaronjeline force-pushed the deprecation/aaronjeline/remove_expr_tags branch from 9d54393 to 08820de Compare October 27, 2023 16:03
cdisselkoen
cdisselkoen approved these changes Oct 27, 2023
View reviewed changes
cedar-policy/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Removed `__expr` escape from Cedar JSON formats
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Removed `__expr` escape from Cedar JSON formats
- Removed `__expr` escape from Cedar JSON formats. (This has been deprecated since before Cedar's first open-source release.)

@aaronjeline aaronjeline merged commit ac78997 into main Oct 27, 2023
5 of 6 checks passed
@john-h-kastner-aws john-h-kastner-aws deleted the deprecation/aaronjeline/remove_expr_tags branch November 14, 2023 16:40
@cdisselkoen cdisselkoen mentioned this pull request Nov 15, 2023
Closed
2 tasks
@sarahcec sarahcec added the 3.0 label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewmwells-amazon andrewmwells-amazon andrewmwells-amazon approved these changes

@cdisselkoen cdisselkoen cdisselkoen approved these changes

@khieta khieta Awaiting requested review from khieta

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

Successfully merging this pull request may close these issues.
4 participants
@aaronjeline @cdisselkoen @andrewmwells-amazon @sarahcec