Skip to content
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

autoreset=True appears not to work #47

Closed
tartley opened this issue Apr 5, 2015 · 8 comments
Closed

autoreset=True appears not to work #47

tartley opened this issue Apr 5, 2015 · 8 comments

Comments

@tartley
Copy link
Owner

tartley commented Apr 5, 2015

reported by email from jason kenny via jonathan hartley (tartley)

If I write a simple program.. ie

import colorama
colorama.init(autoreset=True,strip=False,convert=False)
print colorama.Fore.RED+"hello"
print "some more"
print colorama.Fore.RESET+ "should be back to normal"

and run on windows

test.py > out.txt

I get this in out.txt

←[31mhello
some more
←[39mshould be back to normal

not the expected:

←[31mhello←[0m
←[0msome more←[0m
←[0m←[39mshould be back to normal←[0m
←[0m

Is this known?

@wiggin15
Copy link
Collaborator

wiggin15 commented Apr 7, 2015

This works correctly if you don't use > out.txt and just run the program directly. This is because autoreset doesn't work when there is no tty (which is the case when using output redirection). The commit that added the tty check is here: https://code.google.com/p/colorama/source/detail?r=130c08c1fc5e8df1e3d6e5f97e2f9bf2391afc49&path=/colorama/ansitowin32.py
And this was in response to this issue: https://code.google.com/p/colorama/issues/detail?id=18
I actually don't think the issue in that link is correct -- I expect to see the red and the reset codes in the file just like you do. I think we need to remove this check and send the reset code regardless if there is a tty or not. @tartley what do you think?

@tartley
Copy link
Owner Author

tartley commented Apr 8, 2015

When I forwarded this bug report, I hadn't realised that it was the redirection that was causing the autoreset codes to be omitted. Thanks for catching that @wiggin15!

The aim behind the fix for issue 18 was that Colorama should behave like standard unixy utilities such as 'ls' or 'grep', which only put ANSI color codes in their output if it is going to a terminal. If their stdout is redirected to a file, they do not use color (unless you add extra flags to force them to do so.)

If that is deemed to be desirable behaviour, then:

(a) the omission of the autoreset codes is working as intended, and
(b) perhaps some work needs to be done to make this behaviour more consistent (e.g. perhaps the initial '←[31m' codes should be stripped as well?)

On the other hand, I take your point that it might be simpler to handle this, if at all, at the application level, and colorama should just always print color codes when it is asked to. The downside of that is that lots of diverse applications would have to replicate these 'is_tty' checks (not a biggie really), but also that it's a break from the philosophy of the current behaviour, which I'd be nervous about given Colorama's surprisingly large install base.

@wiggin15
Copy link
Collaborator

wiggin15 commented Apr 9, 2015

Changing the implementation to always print the codes may be a break from the current philosophy, but not from the current behavior. If we fix this (autoreset), then colorama will continue to work as it does now, and will print colors in output redirections, so no applications will break.

btw, here's a quick look at others:

@wiggin15
Copy link
Collaborator

wiggin15 commented Apr 9, 2015

lots of diverse applications would have to replicate these 'is_tty' checks

If we really want, we can add (yet another) flag to initialize, something like only_tty. If it's on, then in case there is no tty, we'll disable convert and enable wrap and strip. This will just be a small convenience for applications that decide they want that.

@tartley
Copy link
Owner Author

tartley commented Apr 10, 2015

Fair enough then. I thought I'd applied this tty check with wider scope than that, but apparently not.

@wiggin15
Copy link
Collaborator

It gets deeper... Long explanation, sorry:

On Windows we also check is_a_tty, because colorama can't convert anything using the Win32 API if there is no console (that's a different reason than the one we discussed. When there is redirection on Windows we can't do conversion, whether we want to or not).
So, when running on Windows with redirection, 'convert' is 'False'. This means that in reset_all, we hit the 'else' clause that checks whether to print a reset code to the stream.
If we remove the is_a_tty check from there, then on Windows with redirection we'll actually print the RESET escape code, which is not desirable.

The proper fix should be to change:

elif not self.wrapped.closed and is_a_tty(self.wrapped):

to

elif not self.strip and not self.wrapped.closed:

Then, when autoreset is on:

  • On Windows with tty, convert is True so we'll reset using the win32 call (same as before the change)
  • On Windows without tty, convert is False and strip is True so we'll do nothing (same as before the change)
  • On Linux with tty, convert is False and strip is False, so we'll print the reset code (same as before the change)
  • On Linux without tty, same thing as above (fixes this issue)

In other words:

  • On Windows, no reset codes are written in both cases
  • On Linux, reset codes are written in both cases

Another note: if init is called on Linux with autoreset=True, strip=True, what should happen? The fix above disables the autoreset in this case, which makes sense because there will be nothing to reset - the escape codes are stripped.

I hope this wasn't too complicated. Am I making sense?

@tartley
Copy link
Owner Author

tartley commented Apr 14, 2015

Lordy. Thank heavens someone has their wits about them. Yep, when I trace through what you wrote carefully, that makes perfect sense.

Linux autoreset=True, strip=True: My gut feel is that strip ought to trump autoreset, so nothing should be printed by the autoreset. My thought-experiment example use case for 'strip' is "my output device doesn't handle ansi codes, and I just want to avoid printing junk characters on the screen."

@wiggin15
Copy link
Collaborator

I pushed the commit to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants