colored-rs / colored

(Rust) Coloring terminal so simple you already know how to do it !
Mozilla Public License 2.0
1.73k stars 83 forks source link

Impl From<String> for ColoredString #126

Closed mahor1221 closed 11 months ago

mahor1221 commented 1 year ago

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

hwittenborn commented 1 year ago

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

mahor1221 commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

Thanks @mahor1221!