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.

Sign up for GitHub

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

Jump to bottom

Prefer implementing Display to From<T> for String #7914

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Prefer implementing Display to From<T> for String #7914

merged 3 commits into from
Oct 24, 2022

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Oct 24, 2022 •
edited
Loading

There’s still impl From<AccountId> for String. It’s left
intentionally as it avoids string allocation when used compared
to using Display.

@mina86 mina86 added P-low Priority: low S-automerge labels Oct 24, 2022
@mina86 mina86 requested a review from a team as a code owner October 24, 2022 14:37
@mina86 mina86 requested a review from akhi3030 October 24, 2022 14:37
@mina86 mina86 requested review from matklad and removed request for akhi3030 October 24, 2022 14:38
@mina86
Copy link
Contributor Author

mina86 commented Oct 24, 2022

@matklad, I think I’d leave AccountId with the implementation but convert the other two. Thoughts?

matklad
matklad approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM with account_id one left in

core/account-id/src/lib.rs Outdated Show resolved Hide resolved
core/crypto/src/signature.rs Outdated Show resolved Hide resolved
@near-bulldozer near-bulldozer bot merged commit 181c28d into near:master Oct 24, 2022
nikurt pushed a commit that referenced this pull request Oct 25, 2022
@mina86
Prefer implementing Display to From<T> for String (#7914)
39e4770 
There’s still `impl From<AccountId> for String`.  It’s left
intentionally as it avoids string allocation when used compared
to using Display.
@mina86 mina86 deleted the b branch November 3, 2022 18:19
nikurt pushed a commit that referenced this pull request Nov 7, 2022
@mina86
Prefer implementing Display to From<T> for String (#7914)
2667e4d 
There’s still `impl From<AccountId> for String`.  It’s left
intentionally as it avoids string allocation when used compared
to using Display.
nikurt pushed a commit that referenced this pull request Nov 9, 2022
@mina86
Prefer implementing Display to From<T> for String (#7914)
afdf1f2 
There’s still `impl From<AccountId> for String`.  It’s left
intentionally as it avoids string allocation when used compared
to using Display.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad approved these changes

Assignees
No one assigned
Labels
P-low Priority: low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mina86 @matklad