-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: updates the emoji ID API to be more idiomatic #6287
feat: updates the emoji ID API to be more idiomatic #6287
Conversation
ced190f
to
2f7e5f2
Compare
Test Results (CI) 3 files 120 suites 37m 39s ⏱️ Results for commit 6762f9e. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 11 suites +11 56m 57s ⏱️ + 56m 57s For more details on these failures, see this check. Results for commit 6762f9e. ± Comparison against base commit 8f1eac6. ♻️ This comment has been updated with latest results. |
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.
Generally when something is constructed from a string the FromStr
trait is more idiomatic.
fn do_something(emoji_id: EmojiId) { ... }
do_something(emoji_str.parse()?);
// equivalent to
do_something(EmojiId::from_str(emoji_str)?);
It may make sense to implement From<PublicKey> for EmojiId
if it isn't already
|
Description
Updates the emoji ID public API to be more idiomatic.
Motivation and Context
The emoji ID public API includes functionality to convert to and from types like
String
andPublicKey
, but not idiomatically. This PR updates the API to useFrom
andFromStr
trait implementations where possible. It also makes some minor changes to checksum function signatures to avoid unnecessary vector cloning, and improves a test.How Has This Been Tested?
Existing tests pass.
What process can a PR reviewer use to test or verify this change?
Check that the new API is, in fact, more idiomatic :)
While this doesn't affect consensus or existing databases, this public API change is technically breaking. For example, existing documentation needs to be updated.