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

dinit-log: Add text color #334

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

TheNofis
Copy link

I just added a couple of lines so that the logging was colored :D

@davmac314
Copy link
Owner

Thanks for opening a PR. To accept this I would require a few changes: since the color sequences are not universal to every terminal, and since some people wouldn't want them anyway, there should be a (documented) way to turn them off (i.e. a command-line argument). Additionally I would like the escape sequences to named macros or constants rather than embed them directly in the message strings.

@mobin-2008 mobin-2008 added Enhancement/New Feature Improving things or introduce new feature A-Importance: Normal C-dinit Things about the main parts of dinit labels May 19, 2024
@@ -0,0 +1,238 @@
# This is the build file for project "dinit"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you pushed everything that was in your git working dir.

Comment on lines +9 to +12
#define COLOR_GREEN "\x1b[32m" // green
#define COLOR_RED "\x1b[33m" // red
#define COLOR_YELLOW "\x1b[31m" // yellow
#define COLOR_RESET "\x1b[0m" // reset
Copy link
Collaborator

Choose a reason for hiding this comment

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

"// green" comment and similar comments are unnecessary, Names are self-explanatory.

@davmac314
Copy link
Owner

Other than cleaning up the PR so that it doesn't include a bunch of built files (as Mobin pointed out):

  • I notice that the same color is used for "FAILED" as "STOPPD" status. I don't think this should be the case. A service that is "stopped" successfully does not indicate an error.
  • the new --color option looks like it is the default anyway so the option has no effect?
  • new option needs documenting in the manual pages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Importance: Normal C-dinit Things about the main parts of dinit Enhancement/New Feature Improving things or introduce new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants