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

Rewrite of EntitiesError #828

Merged
merged 2 commits into from
May 3, 2024
Merged

Rewrite of EntitiesError #828

merged 2 commits into from
May 3, 2024

Conversation

aaronjeline
Copy link
Contributor

@aaronjeline aaronjeline commented May 1, 2024

Description of changes

Rewrite of entities error to bring inline with our general error strategy

Issue #, if available

#645

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 formal model or DRT infrastructure.

@aaronjeline aaronjeline force-pushed the aaronjeline/entities-745-pass branch 3 times, most recently from f587eb6 to 942cef2 Compare May 1, 2024 18:00
@aaronjeline aaronjeline marked this pull request as ready for review May 1, 2024 18:00
cedar-policy/src/api.rs Outdated Show resolved Hide resolved
cedar-policy/src/tests.rs Show resolved Hide resolved
Signed-off-by: Aaron Eline <aeline+github@amazon.com>
@aaronjeline aaronjeline force-pushed the aaronjeline/entities-745-pass branch from 53b06be to 6bdbc91 Compare May 3, 2024 15:51
Signed-off-by: Aaron Eline <aeline+github@amazon.com>
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.

Looks good. We'll probably want to take a second look at the exact structure and placement of sub modules once the whole error refactor is done

@@ -73,8 +73,45 @@ pub mod entities {
self.inner.size_hint()
}
}

/// Errors around entities
pub mod err {
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on #830 maybe we want to move these to err.rs? @khieta

@aaronjeline aaronjeline merged commit a5e4a3a into main May 3, 2024
16 of 17 checks passed
@aaronjeline aaronjeline deleted the aaronjeline/entities-745-pass branch May 3, 2024 18:20
Comment on lines +390 to +395
/// Error type for an expression that is not a record
#[derive(Debug)]
pub struct NotARecord {
/// Expression which is not a record
expr: Box<RestrictedExpr>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I'm late and this has already been merged, but since this is an error type, it would probably make sense for it to derive Error, and have the error message defined here (and ContextCreationError::NotARecord just have error(transparent))

@john-h-kastner-aws john-h-kastner-aws added the breaking-change This is (likely) a breaking change label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This is (likely) a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants