-
-
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
Guard against multiple calls to init(). #236
base: master
Are you sure you want to change the base?
Conversation
BEWARE, I haven't tested this on Windows (because I don't have a Windows box.) It should definitely get tested there before we merge. This should no longer lose the original stdout/stderr, so that we can restore them with a single call to deinit(), while still preserving the behavior that multiple calls to init() can override the passed values for autoreset, convert, strip, wrap. This fix is not intended to reset output to default foreground/background colors after the call to 'deinit()' - that was never intended behavior.
Add an extra test to prove that it's needed.
I confirmed this on Windows 7 with Python 3.5 (yes, it is old, but I think it should be enough). One small concern I have is that passing different parameters to multiple init calls can still change the final effect. For example, if I call |
Please consider releasing a new version, this PR does it for me on Windows 10. |
Thanks for the thoughts. I'll do it any day now... :-) |
Thanks a lot for responding! In my current project I had to resort to some inelegant monkey-patching which I will be more than happy to discard :-) |
@tartley has the day come yet? 😄 I'd like this feature too. May it merged and a new release cut, please? |
This PR would be a big help for some of my use cases....I'd like to build on it a bit so that future calls to init can tweak the autoreset behavior (for use within a context manager). However, I see this one is still open, so curious what the thoughts are on if this will eventually get merged or is there a reason this was never merged and I should perhaps do a new one? |
Is this a dupe of #262 ? Do they both suffer from the same problems wiggin describes above (ie behavior is inadvertantly changed because some calls to init, possibly with different params than last time, are discarded?) |
There is a behavior change yes. But only depending on the order of autoreset. Today, if there is ever a call with autoreset=False, then you can never make a future call where autoreset=True works (since we just keep first init called wins)....I'd want to change that to. Also, not sure if it is important, but each init call re-wraps stdout....the underlying assumption here is that if they want to wrap a new sys.stdout they will now need to call deinit. I admit, I did not notice the other PR, the counter is good as it requires the same number of deinit as init calls. I also liked that this one had some tests 😄 at a quick glance it seems they may work for the other PR too. |
BEWARE, I haven't tested this on Windows (because I
don't have a Windows box.) It should definitely get
tested there before we merge.
This is intended as a fix for:
#145
This PR means that multiple calls to init() should no longer
lose the original stdout/stderr references.
So we can then restore them with a single call to deinit().
It preserves the behavior that multiple calls
to init() can override the passed values for autoreset,
convert, strip, wrap.
This fix is not intended to reset output to default
foreground/background colors after the call to
'deinit()' - that was never intended behavior.