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

Impl From<String> for ColoredString #126

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Impl From<String> for ColoredString #126

merged 1 commit into from
Dec 5, 2023

Conversation

mahor1221
Copy link
Contributor

Complements impl<'a> From<&'a str> for ColoredString.

Copy link
Collaborator

@kurtlawrence kurtlawrence left a comment

Choose a reason for hiding this comment

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

Looks good to me

@hwittenborn hwittenborn force-pushed the master branch 3 times, most recently from 744b5dd to 1921f20 Compare July 3, 2023 11:42
@hwittenborn
Copy link
Member

Why's this needed @mahor1221? Running String::from("blue").blue() locally works just fine, is there some other use case you're needing?

@mahor1221
Copy link
Contributor Author

I frequently use the into() method in my codebase as it makes refactoring easier and I want to maintain consistency and avoid using any alternative methods. For example:

fn paint(vec: Vec<String>) -> ColoredString {
  vec
    .iter()
    .map(|_| todo!())
    .collect::<String>()
    .into()
}

In the code above, if I change ColoredString to another type, such as NewTypeFromColoredString, and this new type implements the From or Into trait, the code should work without requiring additional changes.

@JustAnotherCodemonkey
Copy link
Contributor

Is there anything blocking this? This functionality would be nice and I'm very surprised that this has not happened yet. From<&str> is not good enough since it requires both an additional copy and a new heap allocation where From would simply consume the underlying String into the ColoredString struct, maintaining the allocation and avoiding copying/moving the underlying str data. As @hwittenborn mentioned, it is possible to call any of the ColoredString-creating methods on a String but this simply goes down the Deref to the str underneath which once again results in an inherent heap allocation plus a memcpy for the data. I understand that it's rare that you're creating lots of ColoredStrings from Strings but I looked into this based on a use case where exactly that happens and this extremely small and simple addition of functionality could lead to vastly improved performance on these plaintext String to ColoredString conversions.

@kurtlawrence kurtlawrence merged commit af53167 into colored-rs:master Dec 5, 2023
@kurtlawrence
Copy link
Collaborator

Thanks @mahor1221!

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.

4 participants