-
Notifications
You must be signed in to change notification settings - Fork 1.9k
suppress colorization if stdout is not a tty #494
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
Conversation
@@ -69,7 +78,7 @@ def format(self, record): | |||
# handler and reset the level | |||
logger.setLevel(logging.INFO) | |||
logger.touched = True | |||
ch = logging.StreamHandler(stdout) | |||
ch = logging.StreamHandler(stdout) if sys.stdout.isatty() else logging.NullHandler() |
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.
Why use a NullHandler if the stdout isn't a tty? Don't we still want to send the output?
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 stdout is not a tty, it is not connected to a terminal. which (presumably) means it is actually piped into another script. logging interspersed with actual data is ok for humans, it is very inconvenient for scripts. If the logger actually wrote to a file, then I would have kept it, but since it only writes to stdout that seems neither wise nor convenient. If logging to a file is subsequently added, then I'd be happy to revisit the logic.
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.
So the real intention is to remove all stdout output when not going to a tty, and this just catches most of it (the rest should go through logging too, of course, but just checking).
Does this also mean that e.g. buildozer wouldn't get all the log output when running it via subprocess?
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.
for the remaining cases of explicit calls to distribute.sh that I have seen, buildozer not only doesn't want the log, but it's actually detrimental to its functionning. Maybe the log should be preserved, but it has to go somewhere else than stdout. I am not certain what the best design should be (I am discovering the new code base).
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 that buildozer actually should be able to receive and display the log, even if it can't right now. I'm not worried about the log being preserved if nothing is using it, but we're always asking for buildozer logs to work out what problems people have, and seeing the full verbose log is important for this. That's really why I'm asking, I wasn't sure if buildozer already does this, but it did seem like it should be able to.
I'll merge this change (it's good anyway), but there probably does need to be some way to achieve this. Probably there should be an additional command line argument to toggle it anyway, so buildozer could toggle it on.
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.
Also, just to be clear, a buildozer target for this toolchain would call the p4a executable (or maybe manually call the python), the distribute.sh is only there to give a clear error if people try the old-toolchain method.
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, I am leaning towards calling the python so p4a doesn't have to be "installed".
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.
You can also run toolchain.py file directly, it's the same.
suppress colorization if stdout is not a tty
Thanks. |
suppressing colorization makes the output easier to process by other scripts.