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 From<&EcoString> for EcoString impl #41

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

idlercloud
Copy link
Contributor

Add an impl like impl From<&String> for String to allow convert &EcoString to EcoString. In my use case, this will enable function to take param as impl Into<EcoString>, so it can receive either &EcoString or EcoString.

@laurmaedje
Copy link
Member

I'm not a huge fan of this as it could hide unnecessary clones where a move would be possible. But I guess consistency with std is more important.

@idlercloud
Copy link
Contributor Author

Sorry for accidentally committing irrelevant code. I have revert it.

Since std already did this, it looks ok for us to do the same.

However, I briefly investigated other string optimization crates in the ecosystem, and it seems that none of them implement this.

Considering that cloning EcoString is cheap, for my use case, simply passing an EcoString seems to be acceptable.

So feel free to merge or reject.

@laurmaedje laurmaedje merged commit 20c2337 into typst:main Sep 16, 2024
4 of 8 checks passed
@laurmaedje
Copy link
Member

Let's follow std here rather than other third party crates.

I got to fixing CI so that we don't get a red checkmark on main after merge.

Thanks!

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.

2 participants