Skip to content

Conversation

@khieta
Copy link
Contributor

@khieta khieta commented May 14, 2024

Description of changes

The cause of #862 was the call to remove_empty_lines, which removed empty lines from the formatted policy -- including those in string literals.

Attempt 1: It was simple enough to remove the remove_empty_lines call, but this makes formatted policies ugly since the current code introduces extra newlines around comments. I imagine the reason remove_empty_lines was originally introduced was to handle these extra newlines.

Attempt 2: I tried to get rid of the extra newlines by adjusting how RcDocs are constructed, but that ended up being a pain in the butt.

Attempt 3 (current PR): I added a new function remove_empty_lines_safe, which does some light parsing to avoid removing newlines in strings.

Summary

  • Added the remove_empty_lines_safe function ← the important change
  • Switched to using RcDoc::text in cases where the call to to_string was unnecessary
  • Added comments in some parts of the code that are subtle.

The last two changes are just edits I made while getting familiar with the codebase. I think they're harmless.

We had previously discussed not using RcDoc::text or RcDoc::as_string for string parsing. But I think it’s actually ok: newlines are used in the tests. I think the way to interpret the "must not contain line breaks" guidance is that indentation will not be handled correctly when you do this, which is actually what we want for string literals.

Issue #, if available

Resolves #862

Checklist for requesting a review

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

  • A bug fix or other functionality change requiring a patch to cedar-policy.

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.

@khieta khieta force-pushed the khieta/cedar-862 branch from 6e30b0d to 5d3d326 Compare May 21, 2024 13:27
@khieta khieta marked this pull request as ready for review May 21, 2024 14:34
@khieta
Copy link
Contributor Author

khieta commented May 21, 2024

Marking as ready-for-review since the code is in a reviewable state & CI is passing. I plan on running the formatter fuzzing targets later today -- will add a comment here with the results.

@khieta khieta changed the title [In progress] Fix for #862 Fix for #862 May 21, 2024
@khieta khieta changed the title Fix for #862 Fix formatter handling of blank lines May 21, 2024
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.

Look pretty good.

@khieta
Copy link
Contributor Author

khieta commented May 21, 2024

Ran the formatter target for an hour with no issue. Ran the formatter-bytes target for a few minutes and it immediately found the issue (nice addition @john-h-kastner-aws!). I've fixed the identified bug, and now I'm letting the formatter-bytes target run some more 🙂

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 for fixing it.

@khieta
Copy link
Contributor Author

khieta commented May 21, 2024

Update: formatter-bytes has been running for an hour and a half with no failures, so I'm going to call this good.

shaobo-he-aws added a commit that referenced this pull request May 22, 2024
@khieta khieta mentioned this pull request May 24, 2024
3 tasks
khieta added a commit that referenced this pull request May 24, 2024
shaobo-he-aws pushed a commit that referenced this pull request May 24, 2024
shaobo-he-aws pushed a commit that referenced this pull request May 28, 2024
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.

Policy formatter drops empty lines in multi-line strings

3 participants