-
Notifications
You must be signed in to change notification settings - Fork 286
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
Add --force-color flag #1946
Add --force-color flag #1946
Conversation
Add --force-color flag to force color even when the output is not a TTY.
added force-color flag!!
@jjbustamante (I don't think so this error coming because of the changes I made) sarthak@hp-pavilion:~/pack$ make verify |
here we can see in flags force-color is showing Usage: Available Commands: Flags: |
Could you run |
okk |
ran make verify
add -force-color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is still not working as expected.
I know there are not unit test in this file, but maybe you could also add a new unit test to confirm the new flag is actually doing what you are expecting.
cmd/cmd.go
Outdated
if !canDisplayColor { | ||
color.Disable(true) | ||
if forceColor { | ||
color.Enabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the --force-color
flag, you will notice it will never reach this line. the variable declared on L43 is always set to false
and you are never reading the flag value.
Hint: check L53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the
--force-color
flag, you will notice it will never reach this line. the variable declared on L43 is always set tofalse
and you are never reading the flag value.Hint: check L53
@jjbustamante Thank you, I will check this out!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! just use the fs.GetBool
function to validate if the flag was enabled by the end user. I think you don't need the forceColor
flag anymore, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, that's true there is no longer need to define --force-color explicitly as logic checks directly if the user has provided the --force-color flag using fs.GetBool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait! its fine to remove:
var forceColor bool
but you still need:
rootCmd.PersistentFlags().Bool("force-color", false, "Force color output")
remember, adding the flag, is the only way you have to allow users to set or not the flag, when the user set --force-color
then fs.GetBool("force-color")
will return true.
Also, you used to have:
if force-color {
color.Enabled()
} else {
if no-color {
color.Disable(true)
} else {
// more logic
}
}
but you changed it to:
if force-color {
color.Enabled()
}
if no-color {
color.Disable(true)
} else {
// more logic
}
I think you will have troubles if run pack commands with both --force-color
and --no-color
, I know it is a crazy scenario, but I think the original flow is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh, thanks a lot !! I will take care of this !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sarthaksarthak9.
I apologized for all the back and forth, I think both of us are learning something about it!.
I was checking how the color.Disable(bool)
and color.Enable()
methods works, and I think we can do just one more adjust here.
By default, color is enabled, and we are just disabling it when --no-color
or is not a terminal
, I think what we can do is to do what we are doing today if --force-color
is not the expected behavior. Something like:
if forceColor, err := fs.GetBool("force-color"); err == nil && !forceColor {
if flag, err := fs.GetBool("no-color"); err == nil && flag {
color.Disable(flag)
}
_, canDisplayColor := term.IsTerminal(logging.GetWriterForLevel(logger, logging.InfoLevel))
if !canDisplayColor {
color.Disable(true)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okk, I will try my best
"no longer need to define --force-color explicitly"
correct the logic errors
try to write logic if --force-color is not the expected behavior.
@jjbustamante R the changes still not working yet? |
Hi @sarthaksarthak9 I just tested it and it works!. Here is how I did it:
> true | out/pack build --force-color my-app -p <path-to-samples>/apps/ruby-bundler -b <path-to-samples>/buildpacks/ruby-bundler 2>&1 | cat Even thought I was not able to build the application, we can see in the following image, colors were used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sarthaksarthak9 for your contribution!
Thank you for guiding me |
Add --force-color flag to force color even when the output is not a TTY.
Summary
Output
Before
After
Documentation
Related
Resolves #1839