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

Add Entity::new_with_tags() and Entity::tag() #1402

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Adds public methods Entity::new_with_tags() and Entity::tag() as discussed in #1374.

Discussion points:

  • I made new_with_tags() more generic than the existing Entity::new(); we can take any IntoIterator for the attrs and parents parameters, there's no reason we need to force callers to have HashMap and HashSet specifically. Theoretically I believe we could adjust the Entity::new() signature in the same way without breaking semver if we wanted to keep these aligned.
  • There's also no real reason we need owned String for attribute and tag names; the implementation immediately converts to SmolStr anyway, so we could take any ToSmolStr for instance. I haven't made that change in the current version of this PR because I'm not sure how much we want to keep SmolStr and related types/functions as an implementation detail.

Issue #, if available

Fixes #1374

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.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@john-h-kastner-aws
Copy link
Contributor

john-h-kastner-aws commented Dec 31, 2024

I made new_with_tags() more generic than the existing Entity::new(); we can take any IntoIterator for the attrs and parents parameters, there's no reason we need to force callers to have HashMap and HashSet specifically. Theoretically I believe we could adjust the Entity::new() signature in the same way without breaking semver if we wanted to keep these aligned.

I do prefer the new signature. Changing the old signature would probably break type inference in some edges cases if we want to take a strict definition of semver.

There's also no real reason we need owned String for attribute and tag names; the implementation immediately converts to SmolStr anyway, so we could take any ToSmolStr for instance. I haven't made that change in the current version of this PR because I'm not sure how much we want to keep SmolStr and related types/functions as an implementation detail.

No strong opinion of whether SmolStr should be part of our API, but it doesn't seem like a problem to me. A quick search finds one non-experimental function that already exposes it:

pub fn escaped(&self) -> SmolStr {

@cdisselkoen
Copy link
Contributor Author

I think I'll leave this API using String to keep things a little more consistent between new() and new_with_tags()

@cdisselkoen cdisselkoen merged commit d1f1185 into main Dec 31, 2024
19 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/entity-tags-api branch December 31, 2024 17:49
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.

Add ability to create an Entity with tags in Entity::new()
3 participants