Port buildbot_worker.test.unit.test_scripts_runner.TestOptions.test_version to Python 3 #2390
Conversation
cStringIO is gone on Python 3.
Current coverage is 86.66% (diff: 100%)@@ master #2390 diff @@
==========================================
Files 294 294
Lines 30554 30554
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 26480 26480
Misses 4074 4074
Partials 0 0
|
| if PY3: | ||
| self.stdout = StringIO() | ||
| else: | ||
| self.stdout = BytesIO() |
There was a problem hiding this comment.
it is weird, as the previous implementation implemented stdout as stringio.
There was a problem hiding this comment.
It looks weird, but the old StringIO and cStringIO modules cannot even be imported
in Python 3. io.StringIO deals strictly with Unicode on Python 2, Python3,
and io.BytesIO deals strictly with bytes on Python 2, Python 3.
sys.stdout / sys.stderr are implemented very differently on Python 3 as I mentioned in the comments,
so in order to keep this code working on both Python 2 and Python 3,
using BytesIO on Python 2, and StringIO on Python 3 is necessary.
For this particular case of overriding sys.stdout, it cannot be avoided.
| import errno | ||
| import os | ||
| import re | ||
| import shutil | ||
| import sys | ||
| from io import BytesIO |
There was a problem hiding this comment.
I think this order is OK. isort accepts it.
There was a problem hiding this comment.
I'd say it's NOT OK: imports within a "block" must be sorted by the module name.
There was a problem hiding this comment.
I'd say its ok. the from are placed after the imports within the stdlib import group
| import errno | ||
| import os | ||
| import re | ||
| import shutil | ||
| import sys | ||
| from io import BytesIO | ||
| from io import StringIO | ||
|
|
||
| import mock |
There was a problem hiding this comment.
I'd expect this import to be after the use of future
There was a problem hiding this comment.
Why? isort accepts this.
There was a problem hiding this comment.
Well, mock offers the functionality that is:
- not related to future (=> an empty line)
- more specific to the content of this file (=> move after imports that are more generic)
| self.stdout = cStringIO.StringIO() | ||
| # | ||
| # sys.stdout is implemented differently | ||
| # in Python 2 and Python 3, so we need to |
There was a problem hiding this comment.
why this kind of of formatting? no need to wrap the line here.
There was a problem hiding this comment.
No reason. Does it really matter?
There was a problem hiding this comment.
It does not and it does: a split line for no reason is more difficult to read.
| # override it differently. | ||
| # In Python 2, sys.stdout is a byte stream. | ||
| # In Python 3, sys.stdout is a text stream. | ||
| if PY3: |
There was a problem hiding this comment.
I'd expect this kind of thing to be handled by future, does it not handle it well?
There was a problem hiding this comment.
future doesn't handle this well, because the differences in sys.stdout between Python 2 and Python 3 are quite different and rather tricky.
| import errno | ||
| import os | ||
| import re | ||
| import shutil | ||
| import sys | ||
| from io import BytesIO |
There was a problem hiding this comment.
I'd say it's NOT OK: imports within a "block" must be sorted by the module name.
| import errno | ||
| import os | ||
| import re | ||
| import shutil | ||
| import sys | ||
| from io import BytesIO | ||
| from io import StringIO | ||
|
|
||
| import mock |
There was a problem hiding this comment.
Well, mock offers the functionality that is:
- not related to future (=> an empty line)
- more specific to the content of this file (=> move after imports that are more generic)
|
|
||
| import mock | ||
| from future.utils import PY3 | ||
| from future.utils import string_types | ||
| from twisted.python import log |
There was a problem hiding this comment.
On related note, there should be an empty line before this one.
| self.stdout = cStringIO.StringIO() | ||
| # | ||
| # sys.stdout is implemented differently | ||
| # in Python 2 and Python 3, so we need to |
There was a problem hiding this comment.
It does not and it does: a split line for no reason is more difficult to read.
| # override it differently. | ||
| # In Python 2, sys.stdout is a byte stream. | ||
| # In Python 3, sys.stdout is a text stream. | ||
| if PY3: |
No description provided.