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

Update available colors, remove newline rtrim from ptln, and add new print function #27

Closed
wants to merge 1 commit into from

Conversation

kodie
Copy link

@kodie kodie commented Sep 14, 2021

So let me know your thoughts on this:

This PR implements a few changes:

1.) Update available colors
So as the project is right now, some of the colors are kind of one wouldn't normally expect with terminal colors (like if you normally use https://www.npmjs.com/package/colors in Node), for example, Yellow=Brown, Light Yellow = Yellow, Magenta = Purple, Light Magenta = Light Purple. I updated the colors to match what is defined here: https://www.lihaoyi.com/post/BuildyourownCommandLinewithANSIescapecodes.html

I also added background colors as well as bold, underline, and reversed styles. I understand this is a breaking change but I think it would be worth it.

2.) Remove newline rtrim from the ptln function
Right now the function trims any new line characters from the end of the text making it not possible to add an empty line to text unless you call the function again with an empty string.

3.) Add new print function
I wasn't sure what to call this function, originally I named it format however that doesn't imply that the text will be printed on the screen and I didn't want to have to wrap this function in ptln (which wouldn't even really be possible because that function requires you to define a color). But basically this function allows you to use color codes to combine difference colors easily. For example:

$this->colors->print("<black><redbackground>U</redbackground><whitebackground>S</whitebackground><bluebackground>A</bluebackground></black>");

Instead of throwing an error if someone puts an invalid color code, it just treats it as normal text.

Let me know what you think!

@splitbrain
Copy link
Owner

  • different features should be submitted as different PRs
  • your renaming of color constants will break existing uses (eg. you removed C_LIGHT_GRAY)
  • I'm not sure people expect specific colors associated with different debug levels and I don't see why the mentioned node package should be the "standard" to adhere to. I'd rather not change the behavior for existing users without good reason
  • If there is an official list of color names, I'm happy to have them added
  • I'm not sure what I think about the new print function yet. I want to keep this package simple and parsing custom markup from strings feels slightly too much. It should have unit tests for sure.

@splitbrain splitbrain closed this Sep 15, 2021
@kodie
Copy link
Author

kodie commented Sep 15, 2021

@splitbrain

1.) Totally understandable
2.) We could probably get around this by having some duplicate colors with different names (C_LIGHT_GRAY = C_LIGHT_BLACK, C_BROWN = C_YELLOW)
3 and 4.) I guess there doesn't seem to really be an official list, Wikipedia lists the colors as I stated (https://en.wikipedia.org/wiki/ANSI_escape_code#3-bit_and_4-bit), however the Linux Manual page has them listed as the package already does for the most part (https://man7.org/linux/man-pages/man5/terminal-colors.d.5.html), I think my previous line item would take care of this though. I only mentioned the Node package because it's used by a ton of developers. I could have used CLImate as a PHP example (https://climate.thephpleague.com/styling/colors/)
5.) Again, totally understandable. I could definitely write some unit tests if you're not totally against the function, otherwise, we could just forget it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants