-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
Added check to ensure init() will only have any effect in Windows. #312
Conversation
Massive thanks for this. It does sound like a sensible idea. However, because we have so many dependencies, I'm currently terrified of breaking compatibility with the old behavior. For example, someone somewhere might be relying on the effects of calling init(autoreset=True) on Linux. For context, the reason I say this, the last time I made a release, I built a wheel which didn't pick up the README file for reasons unknown (no doubt my fault somehow), and a Python package without a README is invalid, and won't install, but I didn't notice, and uploaded to PyPI, and within seconds I received a tsunami of github issues, emails and tweets, because I'd broken a million people's "pip install" commands of packages which depend on Colorama.. So... we've got a new release process sketched out, which uploads to test-PyPI and tries installing it from there, but I'm keen to tread very carefully at this point. I haven't merged anything to Colorama, nor made a release, for years. But I'm aiming to do one this week. I might hold off on merging this until I've thought about it more, so it might not make this release. Thoughts welcome. |
Maybe make this part of a major release, then? Compatibility-breaking changes are OK in major releases. Finally bring Colorama to 1.0, since the project is essentially finished anyway, and people will accept that they have to make a conscious decision to upgrade. |
Yeah, that's fair. OK then, I'll aim for that... |
Thanks once again. This change sounds sensible but needs some thought.
|
Thanks heaps for this. FYI, an alternative approach has been mentioned. Instead of modifying the behavior of the existing 'init()' call, upon which some people may be relying, the idea has been floated to add a new method alongside 'init', which does something like what you (and others) require. For details, please see the final final comment my njsmith here ("I would like to sneak one more feature in") , just before I merged. |
On reflection, to be explicit, I'm going to close this PR in favor of the approach suggested by njsmith, with the intention of getting a relatively trivial PR merged which can do this, and then making a release. |
Much like #304, I am also experiencing issues because of the not-quite-true nature of that line from the docs that carltongibson quoted.
This is untrue when running in Linux within a Docker container, because even if you don't pass
strip=True
toinit()
, it'll still act like you did because in Docker,sys.stderr
isn't a tty. It's a file descriptor that prints to a special log file which you can follow to get output that looks like a tty, butisatty()
returns False. This forcesstrip=True
, do to theif strip is None
check in theAnsiToWin32
constructor, which activates theAnsiToWin32
wrapper even though the code isn't running on Windows. This completely breaks the colored output of all logs coming out of the Docker container.This pull request makes it 100% certain that Colorama won't screw around with
sys.stderr
orsys.stdout
unless the program is actually running on Windows. Libraries like StructLog already wrap their call tocolorama.init()
in such a check, so I figure that Colorama itself should be doing this.