-
Notifications
You must be signed in to change notification settings - Fork 81
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
True colors #6
Comments
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:
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 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 |
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) ? |
I'll spend some time on that! Thanks for the feedback!
The first version adds a variant to the pub enum Color {
// ...
True(u8, u8, u8),
Palette(u8),
} and this has the consequence that 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 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 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 <notifications@github.com> 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
<#6 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEIIC4UZU3FDAUWYVAVKFLRM2MGRANCNFSM4CIOSIAQ>
.
|
In the first version, the only API change is the fact that 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 let mut string = String::new();
string.push_str(&Color::Blue.to_fg_str()); The second version doesn't change the |
Update: as suggested by @kurtlawrence in #78, I changed the output type of |
Implements:
The text was updated successfully, but these errors were encountered: