Non-erroring constructor for `PolicyId`

[john-h-kastner-aws]

Category

User level API changes

Describe the feature you'd like to request

The PolicyId struct is currently only constructed Using PolicyId::from_str. This function returns a Result, but our documentation states that it "currently always returns Ok()." This encourages calling unwrap() on our Results, which in many other cases will result in a panic. I think we should commit to this API and make this from_str Infallible. Changing the error type would be breaking change, so we should also release a PolicyId::new(String) constructor in a minor version.

I don't recall ever considering a design where we might limit valid PolicyId strings, and adding a restriction at this point would be serious braking change. If we later decide that it should error, we can make the constructors error again as a breaking change in a major version, but making a valid PolicyId an error should already be treated as a breaking change. Being forced to change the function call would make it more obvious that a change occurred.

Describe alternatives you've considered

.

Additional context

No response

Is this something that you'd be interested in working on?

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

[cdisselkoen]

+1 to this issue. I think we should keep the FromStr implementation but have the error type be Infallible; there's precedent in #372 for Eids.

If we make a separate PolicyId::new(), we could have it take Into<SmolStr> instead of String -- a small generalization that could probably enable more efficiency if the caller has &str (avoiding creating a String just to copy it into a SmolStr). Alternately we could just take &str, which would always copy, and lose efficiency only if the caller has exactly a SmolStr (I think)

[cdisselkoen]

Alternately, #553 suggests taking AsRef<str> which would also be fine, having performance characteristics similar to taking &str I think, but potentially more ergonomic. Regardless, we should do something consistent between this issue and #553