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

feat: Implementation of From<Cow<'static, str>> for StyledStr #5799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zaira-bibi
Copy link

@zaira-bibi zaira-bibi commented Nov 1, 2024

Implemented From<Cow<'static, str>> for StyledStr under #5785.

(I'm not sure where I was supposed to write the tests for it, so guidance in that would be really appreciated. Thanks!)

@epage
Copy link
Member

epage commented Nov 4, 2024

In #5785 (comment) I asked for:

If we have other places where there is a From from either 'static or owned, we should add it there as well.

Have those been checked for? For example, Str, OsStr, Id

@zaira-bibi
Copy link
Author

@epage I've added it for Id. For Str and OsStr, I am not sure if it's supposed to be for feature="string". Would appreciate some help, thanks!


impl From<Cow<'static, str>> for StyledStr {
fn from(value: Cow<'static, str>) -> Self {
StyledStr(value.to_string())
Copy link
Member

Choose a reason for hiding this comment

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

Please use .into_owned()

to_string can work on any display and can make code more sloppy on refactor.

(sorry, I overlooked this last time and apparently a clippy lint didn't catch this case)

Choose a reason for hiding this comment

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

No, Cow<'static, str>::to_string will cause unnecessary clone on owned case, into_owned is indeed what we want.

@epage
Copy link
Member

epage commented Nov 6, 2024

, I am not sure if it's supposed to be for feature="string". Would appreciate some help, thanks!

Without string, we can only store Cow::Borrowed and would have to panic on Cow::Owned. It would be better for us to not support it and instead provide a compiler error.

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.

3 participants