-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
add safety wrapper around sys.stdout assignment #724
Conversation
Looks okay, but make sure to add news fragment, and add issue it solves in commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! Definitely agree with the idea, just a few tiny things to fix up and then we can merge this :)
tox/venv.py
Outdated
redirect=self.session.report.verbosity < 2) | ||
sys.stdout = old_stdout | ||
try: | ||
sys.stdout = codecs.getwriter('utf8')(sys.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be outside of the try
I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 69e05ac
tox/venv.py
Outdated
self._pcall(argv, cwd=self.envconfig.config.toxinidir, action=action, | ||
redirect=self.session.report.verbosity < 2) | ||
except Exception: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove these two lines, they do nothing try: finally:
is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in 69e05ac
Codecov Report
@@ Coverage Diff @@
## master #724 +/- ##
=======================================
Coverage 94.93% 94.93%
=======================================
Files 11 11
Lines 2427 2427
=======================================
Hits 2304 2304
Misses 123 123 Continue to review full report at Codecov.
|
@gaborbernat added changelog entry in b33d6d5 Anything else required for this PR? |
See #723 for details on why this PR is being raised
Contribution checklist:
(also see CONTRIBUTING.rst for details)
in message body
<issue number>.<type>.rst
for example (588.bugfix.rst)<type>
is must be one ofbugfix
,feature
,deprecation
,breaking
,doc
,misc
CONTRIBUTORS
(preserving alphabetical order)