Respect PY_COLORS variable #1533
Conversation
|
Nice one @ssbarnea! Any way to test this also? |
|
I don't know an easy way to test it for CI but you can easily run Because |
|
BTW, do you know a way to make the signing automated on git, the DCO stuff is a real PITA. |
ssbarnea
force-pushed
the
color
branch
2 times, most recently
from
feb1383 to
6735860
Compare
|
@decentral1se did you register @lwm as a backup account? if yes, you might want to add a status/bio there pointing to your new handle... |
molecule/logger.py
Outdated
| return True | ||
| if os.environ.get('PY_COLORS') == '0': | ||
| return False | ||
| return hasattr(file, 'isatty') and file.isatty() and \ |
There was a problem hiding this comment.
It's more pythonic to use brackets for multiline expressions, using EOL escaping is discouraged in typical pythonic style guides.
Also, why do you need to check whether file.isatty() method is present? Isn't that guaranteed?
There was a problem hiding this comment.
After looking closer I realized that you use file() builtin which is a deprecated alias for open(). So you basically ignore this check under Python 3 and always disable colors under it, which is probably not what you want.
Moreover, file itself doesn't really point to any io stream: you should be checking stdout stream.
Here's what you should be doing instead:
| return hasattr(file, 'isatty') and file.isatty() and \ | |
| return sys.stdout.isatty() and \ |
There was a problem hiding this comment.
I will replace use of reserved file with "fh" as a file-handle is what it can receive. I wonder why our linting does not do anything about that. This is something we should fix inside "lint".
There was a problem hiding this comment.
I think you cannot lint this because linters except for pylint do only static analysis + in some version file builtin exists. What do you mean by fh?
ssbarnea
force-pushed
the
color
branch
3 times, most recently
from
7bc29b6 to
f8a0b1b
Compare
test/unit/test_logger.py
Outdated
|
|
||
| def test_markup_detection_tty_no(monkeypatch): | ||
| monkeypatch.delenv('PY_COLORS', raising=False) | ||
| assert not logger.should_do_markup(FileHandleTTY(False)) |
There was a problem hiding this comment.
According to pytest practices, it is typical to use fixtures for such things.
You could do
@pytest.fixture
def not_tty():
return FileHandleTTY(False)
def test_markup_detection_tty_no(monkeypatch, not_tty):
molecule/logger.py
Outdated
| os.environ.get('TERM') != 'dumb') | ||
|
|
||
|
|
||
| def colorama_init(): |
There was a problem hiding this comment.
Can we test this method in conjunction with the other one, please?
Because currently it looks like
|
This pull request was moved and moved since nearly five months. Anything we can to bring it forward? |
|
@xoxys I'm happy to accept it once the harmful code is eliminated. I'd prefer to see some regression/integration test included as well, though. |
ssbarnea
force-pushed
the
color
branch
2 times, most recently
from
bdbd3ec to
b2a837e
Compare
Yeah, apologies about that @xoxys. If you makes you feel any better, I also had to wait 5 months (and endure 99 comments!) for #1458 ... :) but it's just due transition and https://molecule.readthedocs.io/en/latest/contributing.html#move-to-red-hat etc. We're much improved lately, I think. Just needs concerns in #1533 (comment) addressed! |
|
@decentral1se no need to apologize for anything. You all do a great job for the open source community! I am currently moving to a new apartment so there is not much time to do something by my own. But i would love to see this feature in the next time :) |
| import sys | ||
|
|
||
| import colorama | ||
| from ansible.module_utils.parsing.convert_bool import boolean as to_bool |
There was a problem hiding this comment.
Do you mind putting this import one line above to maintain alphabetic order?
decentral1se
added
the
test
|
Inclined to push this on the v2.20 release! The original report talked about improving the readability of Molecule CI logs and I think that is definitely something we want sooner than later. I'll merge later on (assuming CI passes) today unless I hear otherwise. |
Allow user to define PY_COLORS=1 or 0 to force use or not use of ANSI coloring sequences. Use of this option overrided default behavior of detecting TTY status of stdout and allows users to control its use CI systems which may support ANSI but where there is no TTY. Implementation based on https://github.com/pytest-dev/py/blob/master/py/_io/terminalwriter.py#L131 which is also used by tox and many other similar tools. Fixes: ansible#1510 Signed-off-by: Sorin Sbarnea <[email protected]>
|
This was much needed in the end! See ansible/ansible-runner#200 👍 |
Allow PY_COLORS to control coloring support.
Allow user to define PY_COLORS=1 or 0 to force use or not use of
ANSI coloring sequences.
Use of this option overrided default behavior of detecting TTY status
of stdout and allows users to control its use CI systems which
may support ANSI but where there is no TTY.
Implementation based on https://github.com/pytest-dev/py/blob/master/py/_io/terminalwriter.py#L131
which is also used by tox and many other similar tools.
Fixes: #1510