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

Cows. Cows everywhere. #135

Open shanecelis opened 1 year ago

shanecelis commented 1 year ago

Great project. I saw an opportunity to alloc less. This PR changes input from a String to a Cow<'a, str> and the implementation for &'a str to Into<Cow<'a, str>> which will cover &str as before and String and Cows and makes the necessary changes to support that. No usage change for the end user, just broader support and less allocs.

As an aside, I added print statements to the dynamic_colors example.

shanecelis commented 1 year ago

I reverted the changes that weren't meant to go into this PR (removed a struct called ColoredStrings). And I found that tests weren't working when I varied between "cargo test", "cargo test --all-features", and "cargo test --no-default-features". It seemed like the path of least resistance was to change the negating feature "no-color" to a default feature of "color", which is what Cargo recommends probably so things like "all-features" will work. I apologize for if this is intrusive or presumptuous.

hwittenborn commented 1 year ago

Thanks for the PR @shanecelis! I've been invited on as a contributor to overlook some things, and I'll get this PR look at once I get a bit more familiar with the codebase. I just need to get a better grasp on how everything's working internally, and then I'll be able to look over this code and give it a good review.

hwittenborn commented 1 year ago

Hey @shanecelis, there's plans for a v3 of colored where #139 would be implemented, and because of that I'm probably going to hold off on getting this merged.

Once that issue gets implemented, it should avoid any allocations entirely, which this PR seems to aim for but not entirely achieve. I do like the idea of allocating less, but I'm not positive it'll make much of a difference in the actual speed/memory usage of a program. Would you have any metrics for that?

kurtlawrence commented 11 months ago

154 intends to expose the internals of ColoredString, with the intention of having mutability of the underlying string.

A common pattern seems to be having the colouring and styling orthogonal to the backing string.

I think a good way forward would be to create a generic structure which is agnostic to the backing string, such as

struct Colored<T> {
    input: T,
    fgcolor: Option<Color>,
    bgcolor: Option<Color>,
    style: style::Style,
}

And then implement the required conversions, dereferencing, and formatting agnostic to the backing type.