-
Notifications
You must be signed in to change notification settings - Fork 60
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
Refactoring in api.rs #830
Conversation
/// Construct an [`EntityTypeName`] from the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn new(ty: ast::Name) -> Self { | ||
Self(ty) | ||
} |
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.
New function added by this PR
cedar-policy/src/api/id.rs
Outdated
/// Construct an [`EntityUid`] from the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn new(uid: ast::EntityUID) -> Self { | ||
Self(uid) | ||
} | ||
|
||
/// Deconstruct an [`EntityUid`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn into_inner(self) -> ast::EntityUID { | ||
self.0 | ||
} | ||
|
||
/// Deconstruct an [`EntityUid`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn as_inner(&self) -> &ast::EntityUID { | ||
&self.0 | ||
} |
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.
New functions added by this PR
cedar-policy/src/api/id.rs
Outdated
/// Construct a [`PolicyId`] from a source string | ||
pub fn new(id: impl AsRef<str>) -> Self { | ||
Self(ast::PolicyID::from_string(id.as_ref())) | ||
} | ||
|
||
/// Deconstruct an [`PolicyId`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn into_inner(self) -> ast::PolicyID { | ||
self.0 | ||
} | ||
|
||
/// Deconstruct an [`PolicyId`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
pub(crate) fn as_inner(&self) -> &ast::PolicyID { | ||
&self.0 | ||
} |
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.
New functions added by this PR
/// Deconstruct an [`Expression`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
#[cfg(test)] | ||
pub(crate) fn into_inner(self) -> ast::Expr { | ||
self.0 | ||
} |
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.
New function added by this PR
/// Deconstruct an [`RestrictedExpression`] to get the internal type. | ||
/// This function is only intended to be used internally. | ||
#[cfg(test)] | ||
pub(crate) fn into_inner(self) -> ast::RestrictedExpr { | ||
self.0 | ||
} |
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.
New function added by this PR
Marking this as ready for review now that (most of) the CI has passed. I don't think the Java failures are the fault of this PR -- I think we just need to review & merge cedar-java#133. Will re-run CI once that PR is merged. |
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.
Definitely approve of the idea, making api.rs more organized.
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.
We should add allow
directives for the new clippy lints if we're OK with them.
Latest updates:
The only potentially contentious change is this last one. Because |
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.
LGTM
Description of changes
This PR performs some refactoring to reduce the size of our api.rs file. For the most part, the changes are just copy-and-paste, but I did need to add some new
pub(crate)
functions to handle the new layer of indirection. I’ll add review comments to any new functions added.EntityUid
andPolicyId
to a separate api/id.rs file.The new files are included in api.rs using:
I believe this leaves the API unchanged from a user's perspective. Lmk if I'm wrong about that.
The changes in this PR introduce two new clippy warnings:
PolicyId
andEntityId
since these types end with "id" (the name of the containing module). I think we can safely ignore these because the types arepub use
d in api.rs, so users should writecedar_policy::PolicyId
instead ofcedar_policy::id::PolicyId
.fold_partition
function. The reason I made itpub(crate)
instead of private is that it's also used in testing code. I think it is still effectively private with the current setting.Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
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):