-
Notifications
You must be signed in to change notification settings - Fork 103
Parse trailing commas in Cedar policy files #1606
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
Parse trailing commas in Cedar policy files #1606
Conversation
695ee4a
to
33588fb
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 100.00% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.75% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 100.00% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.75% Status: PASSED ✅ Details
|
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 100.00% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.75% Status: PASSED ✅ Details
|
@@ -84,8 +84,9 @@ match { | |||
} | |||
|
|||
Comma<E>: Vec<E> = { | |||
<e:E?> => e.into_iter().collect(), | |||
<mut es:(<E> ",")+> <e:E> => { | |||
=> vec![], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason why we have special cases for 0 and 1 elements?
As opposed to defining the macro as:
Comma<E>: Vec<E> = { // (1)
<mut v:(<E> ",")*> <e:E?> => match e { // (2)
None => v,
Some(e) => {
v.push(e);
v
}
}
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely not :) Your version is a lot more concise so I'll change to that - thanks!
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 100.00% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.74% Status: PASSED ✅ Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks
Partially implements RFC 71 by adding parser support for trailing commas to the policy parser. Part of cedar-policy#1605 Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
17d7654
to
15bf6d0
Compare
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 100.00% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.92% Status: PASSED ✅ Details
|
Signed-off-by: Mats Jun Larsen <mats@jun.codes> Signed-off-by: Mish Jude <mishiej@amazon.com>
Signed-off-by: Mats Jun Larsen <mats@jun.codes>
Description of changes
Partially implements RFC 71 by adding parser support for trailing commas to the policy parser. I will take care of the schema parser in another PR.
Test diagnostics have been updated where the change in grammar caused the expected token set to change. I have also added simple tests for the cases where trailing commas occur in the policy files.
Issue #, if available
Part of #1605
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy
(e.g., changes to the signature of an existing API).cedar-policy
(e.g., addition of a new API).cedar-policy
.cedar-policy-core
,cedar-validator
, etc.)I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec
(choose one, and delete the other options):cedar-spec
, and how you have tested that your updates are correct.)cedar-spec
. (Post your PR anyways, and we'll discuss in the comments.)I confirm that
docs.cedarpolicy.com
(choose one, and delete the other options):cedar-docs
. PRs should be targeted at astaging-X.Y
branch, notmain
.)This change requires updates in the grammar shown in the docs https://docs.cedarpolicy.com/policies/syntax-grammar.html. The changes are already outlined in the RFC itself. I believe? the docs repo is still private so somebody at AWS will have to do this.