True colors

Implements:

"hello".palette(23); // 23nth colour of palette
"hello".true_colour(23, 45,  23); // true colour code
"hello".hex_colour("#232332"); // hex colour code

Hi,

first of all, thank you for this nice library! It's very simple to use and allow us to produce magnificent output!

I've started working on the true color feature on my fork.

For the moment, I added:

• true_color and on_true_color methods that take three u8 as parameter. However, since those parameters are dynamic, I feel like I have no other choice but to make to_fg_str and to_bg_str return Strings instead of &strs.

• hex_color and on_hex_color. In your original post, you used it as a string, like "hello".hex_color("#232332") but it felt simpler to take as parameter a u64, which allows a user to use it like "hello".hex_color(0x232332). This ensures that the parsing will never fail, and I feel like it's not much different.

I also added a small example that looks like this on my computer:

What do you think of it?

It would be great to see this merged in!

---

For those trying to use the fork version in the meantime, here's example syntax of referencing the dependency from github:

[dependencies]
colored = { git = "https://github.com/tforgione/colored.git", branch = "master" }

Hi again,

I was not really satisfied with making to_fg_str and to_bg_str returning Strings, so I now have another implementation that you might want to check. It uses an enum (that I named AllColor which is a poor name but I didn't find anything better, if you have ideas, please let me know) that can be either a standard color or a true color.

Then, I updated the accoding functions in order to make things work, and added the same example.

Feel free to tell if you want me to submit a pull request !

Hi,

the latest commit on my second fork adds the palette and on_palette functions. It also adds an example that looks like this on my computer:

Hi @tforgione ! Thanks you for your work, indeed I would like a PR ! This would bring great value to colored ! :)

Moreover, per my repository policy, any non-trivial merged PR will make you collaborator of this repository, which enable you to contribute to PR reviews and PR merging.

In order to have a not only a good, but a great PR, I would like more documentation (especially in the README), and tests. None of your logic is tested which is an issue.

What's the difference between your first fork (colored) and the second (colored-2) ?

In order to have a not only a good, but a great PR, I would like more documentation (especially in the README), and tests. None of your logic is tested which is an issue.

I'll spend some time on that! Thanks for the feedback!

What's the difference between your first fork (colored) and the second (colored-2) ?

The first version adds a variant to the Color enum like so:

pub enum Color {
    // ...
    True(u8, u8, u8),
    Palette(u8),
}

and this has the consequence that to_fg_str and to_bg_str can no longer return &str, because I need to dynamically construct the String depending on the values in the variant.

The second version defines a new enum like so (names can be changed):

pub enum AllColor {
    Color(Color),
    True(TrueColor),
    Palette(u8),
}

and keeps your Color struct unchanged. It then changes the types in ColoredString and adds some pattern matching in compute_style.

I'm not sure about what is the best, and now that I think about it, it seems to me that the first version is simpler.

Really like this, any possibility of merging?

Bumping for interest :)

@tforgione I saw both your forks. Are they stable? Are you pushing them to crates.io?

@christoshadjiaslanis I didn't really want to push to crates, because I thought doing a PR would be a better, but since it has been a while since I made those forks, I guess publishing would be the best thing to do, so in the midtime, you can use recolored which is a published version of my first fork, which I think is the cleanest.

In terms of stability, I guess it should be fine, I just merged the upstream updates, everything compiles and the tests and examples run without issues.

@mackwic as I said, the first fork seems to be the cleanest to me. I also see that I improved the README and added some tests, though I didn't comment back on this issue. If you have some time, maybe you can check it out to see if I can submit a PR. The default branch of the fork is recolored, with the modified Cargo.toml and README.md so that people that end up on my github can use the forked version. Of course, for the PR, I would use my master branch, which has your original Cargo.toml and README.md.

Thanks again :)

I would be happy to accept your PR. You are wanting to put the work to it, and that's precious ! Of course, the API needs not to change too much, but hey that's also part of life. Of the api changes too much, we will need an intermediary version with both apis and deprecation message so that our users would be able to make the change without breaking anything.

Thank you for your time, Thomas. I should add you as a maintainer of the crate on crates.io so that you should be able to push. I don't have time right now, I'll add a reminder for this weekend but if I forget, don't be afraid to ask and remind me.

Best, Thomas

Le jeudi 16 avril 2020, Thomas Forgione [email protected] a écrit :

@christoshadjiaslanis https://github.com/christoshadjiaslanis I didn't really want to push to crates, because I thought doing a PR would be a better, but since it has been a while since I made those forks, I guess publishing would be the best thing to do, so in the midtime, you can use recolored https://crates.io/crates/recolored which is a published version of my first fork, which I think is the cleanest.

In terms of stability, I guess it should be fine, I just merged the upstream updates, everything compiles and the tests and examples run without issues.

@mackwic https://github.com/mackwic as I said, the first fork seems to be the cleanest to me. I also see that I improved the README and added some tests, though I didn't comment back on this issue. If you have some time, maybe you can check it out to see if I can submit a PR. The default branch of the fork is recolored, with the modified Cargo.toml and README.md so that people that end up on my github can use the forked version. Of course, for the PR, I would use my master branch, which has your original Cargo.toml and README.md.

Thanks again :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mackwic/colored/issues/6#issuecomment-614430011, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEIIC4UZU3FDAUWYVAVKFLRM2MGRANCNFSM4CIOSIAQ .

@mackwic

Of course, the API needs not to change too much, but hey that's also part of life. Of the api changes too much, we will need an intermediary version with both apis and deprecation message so that our users would be able to make the change without breaking anything.

In the first version, the only API change is the fact that to_fg_str and to_bg_str return Strings instead of &strs. I guess it leads to very few breaks in exisiting code. For example, one break that will occur is the following:

let mut string = String::new();
string.push_str(Color::Blue.to_fg_str()); // works in the current version, will not work anymore

which would be really weird and wouldn't have the expected behaviour anyways. Anyways, if someone has a reason to do that, it can simply be fixed by adding a & in front of the argument since &String can coerce to &str:

let mut string = String::new();
string.push_str(&Color::Blue.to_fg_str());

The second version doesn't change the Color enum, so this particular breaking change won't happen.

Update: as suggested by @kurtlawrence in #78, I changed the output type of to_fg_str and to_bg_str to Cow<str> to avoid unnecessary allocations.