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

Optional command color #1670

Merged
merged 19 commits into from
Oct 11, 2023
Merged

Conversation

avi-cenna
Copy link
Contributor

@avi-cenna avi-cenna commented Sep 9, 2023

Draft MR which attempts to resolve #1186, ie allowing for colored command output. I can add to the list of available colors, but was hoping to get a quick review first to make sure my approach is sensible.

@avi-cenna
Copy link
Contributor Author

@frederikhors Do you mind doing a quick review of this draft MR to enable colored output for recipe lines? You were the one who originally opened the issue, and I figure you might be a little more experienced with Rust than I am.

@avi-cenna avi-cenna changed the title Feature/1186 command color Enable optional command color Sep 9, 2023
@avi-cenna avi-cenna changed the title Enable optional command color Optional command color Sep 9, 2023
@frederikhors
Copy link

I'm sorry. I'm not using it anymore.

@avi-cenna
Copy link
Contributor Author

@frederikhors , no problem at all, and thank you for your reply. If I may ask, what do you use now instead of just?

@poliorcetics
Copy link
Contributor

I was going to create an issue for this but there already is an MR, thanks @avi-cenna !

A question: why the limitation to cyan and purple instead of all the "base" colours usually found in terminal emulators ?

@avi-cenna
Copy link
Contributor Author

@poliorcetics I wanted to start off with just a couple colors until someone could review and confirm that my approach was reasonable. Thank you for the review. Are you a maintainer on this project?

Copy link
Owner

@casey casey 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, added a bunch of comments. We should also add all the rest of the colors that ansi_term supports.

src/color.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/color.rs Outdated Show resolved Hide resolved
@avi-cenna
Copy link
Contributor Author

@casey thanks for the review. I have applied your suggestions as much as possible, and added the additional colors. I've also added one integration test for the command-color argument.

@avi-cenna avi-cenna requested a review from casey October 10, 2023 22:41
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Nice, looks great! I made a couple changes: I sorted the command color values alphabetically and reworded the help text message. Merging now!

@casey casey enabled auto-merge (squash) October 11, 2023 00:02
@casey casey merged commit 22e1033 into casey:master Oct 11, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants