Skip to content

Conversation

john-h-kastner-aws
Copy link
Contributor

Description of changes

Fix #787 and a similar comment reordering issue I found for if-then-else expressions.

Based on the existing tests and some time running the formatter fuzz target locally, I'm fairly confident that not adding end_comment in VariableDef won't break anything, but review is of course appreciated.

As a side effect of the VariableDef change, a line break is introduced in the policy scope for some policies in test.cedar. I don't understand how that was introduced, but I'm willing to accept it.

Checklist for requesting a review

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

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new 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 formal model or DRT infrastructure.

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. Thanks a lot.

@shaobo-he-aws
Copy link
Contributor

It'd great if we can update the fuzzing target to test that there are no reorderings.

@john-h-kastner-aws
Copy link
Contributor Author

It'd great if we can update the fuzzing target to test that there are no reorderings.

Found the if reordering bug after doing just that. Will post next week.

Cargo.lock
Cargo.lock

# Generated by `insta` on failing test case. Should run `cargo insta review`

Choose a reason for hiding this comment

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

What's this from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #856

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.

Cedar formatter reorders comments
4 participants