use colorama to support ANSI color codes on Windows #296
Conversation
|
This adds an optional dependency on colorama on Windows for colored stdout reports. |
There was a problem hiding this comment.
Really good change!
Should we add this to setup.py, so that it's also installed automatically on Windows? (note that I'm using .extend(), just in case we have more Windows-specific dependencies in the future, for a single item, .append() would of course also work)
After the line:
m['install_requires'] = ['minidb', 'PyYAML', 'requests', 'keyring', 'pycodestyle', 'appdirs', 'lxml']
Add this:
if sys.platform == 'win32':
m['install_requires'].extend(['colorama'])
|
Thanks! Windows dependencies added in |
lib/urlwatch/reporters.py
Outdated
| self._has_color = sys.stdout.isatty() and self.config.get('color', False) | ||
| if sys.platform == 'win32' and self._has_color: | ||
| import colorama | ||
| colorama.init() |
There was a problem hiding this comment.
Looking at the code in https://github.com/tartley/colorama/blob/master/colorama/initialise.py#L23, it looks like the colorama module doesn't support calling init() multiple times (well, at least it will probably re-wrap the streams or something, and "uniniting" might no longer be possible).
I've filed tartley/colorama#205 in colorama, maybe it can/should be fixed there (too?).
For now (and to support old installations of the colorama module), maybe add a global/static member _colorama_inited and only import and call colorama.init() once.
There was a problem hiding this comment.
I've added a static member _colorama_inited as requested. But I'm afraid it's a bit futile given this flaw in colorama.
I noticed that, even without this patch, it's possible to get proper colored stdout report on Windows by just having a browser job, if colorama is already installed. This is because, BrowserJob imports requests_html which imports pyppeteer which imports tqdm which imports colorama (if available) and calls init().
I really don't know how to properly handle this situation, or if it's even meaningful to handle it, when any package in urlwatch's dependency tree can potentially call colorama.init(), or user may write hooks.py that calls it.
Earlier related issues: tartley/colorama#145, tartley/colorama#128, tartley/colorama#182
There was a problem hiding this comment.
Super dirty idea: We call colorama.init() outside of the reporter, somewhere in urlwatch's main function (before any other code like imports or hooks gets run) and then we monkey-patch the module to have a no-op init function?
colorama.init()
def dummy_init(*args, **kwargs):
...
colorama.init = dummy_init
There was a problem hiding this comment.
Hmmm... dirty it is.
This works for the current situation. However, it wouldn't work if imported modules do something like from colorama import init.
There was a problem hiding this comment.
Oh... of course it works for from colorama import init. What was I thinking...
But then what do we do about colorama.deinit? What if external code called init (nothing happens) then called deinit (color gone), after that StdoutReporter starts working and we have no color and a mess of ANSI codes! (Windows only of course)
Or... what if someone uses urlwatch as a library, then the main function has no effect... Aggghhh!
How about we don't call init and don't change sys.stdout, and just directly use colorama.AnsiToWin32 inside of StdoutReporter.submit (like one example in colorama's README)?
This way, other modules can do whatever they like, we just ensure that StdoutReporter.submit has the right color, and no side effect on sys.stdout.
Directly use `colorama.AnsiToWin32` in `StdoutReporter` with a custom `print` function. Avoid issues with `colorama.init`. See tartley/colorama#205.
|
Ready to be merged once the merge conflict (due to merging of the |
close #295