-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Support possibility to wrap either stdout or stderr #257
base: master
Are you sure you want to change the base?
Support possibility to wrap either stdout or stderr #257
Conversation
Hey. FYI, Yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it! |
I think I would prefer passing the streams directly instead of as strings such that the signature would be |
The only valid values for the stream argument would be either |
if sys.stdout is None: | ||
wrapped_stdout = None | ||
else: | ||
elif wrap != "stderr": |
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.
Forgive my very overdue drive-by review, but does this do the wrong thing if wrap==False ?
if sys.stderr is None: | ||
wrapped_stderr = None | ||
else: | ||
elif wrap != "stdout": |
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.
same as above
Also: This PR changes the code and behavior, but does not include corresponding test changes, which it would need before it could be merged. (or how else would future contributors know when their changes break your code?) (I thought I mentioned this last night, but don't see it now. Perhaps I messed up.) |
Thanks for the comments, I'll review them on weekends |
It's common to use
stdout
to generate binary files andstderr
to print log and errors. At the same timecolorama
provides very convenient functionality to strip color codes from streams when output is redirected. The problem is that colorama wraps bothstdout
andstderr
which strips binary symbols fromstdout
when binary files are generated.It would be very useful for colorama library to wrap separately
stderr
/stdout
on demand during initialization. I suggest to reuse 'wrap' argument tocolorama.init()
like this:This pull request is linked to issue #256