Conversation
don't think so, Python>=3.3 is fine
why? |
|
@chekunkov, I see 2 main reasons: first because we lose the option to specify a native string literal (or at least it is most more cumbersome [1]). It may be useful in the future. Also, Armin Ronacher (flask) started a nice discussion about [1] http://python-future.org/unicode_literals.html#drawbacks |
|
I see, thanks @bertinatto |
hubstorage/batchuploader.py
Outdated
| def _encode_identity(iter): | ||
| data = StringIO() | ||
| data = BytesIO() | ||
| for item in iter: |
There was a problem hiding this comment.
sorry for nitpicking, but can we replace iter with something like iterable here - I don't like when builtins are shadowed
There was a problem hiding this comment.
Sure. Done.
I handled it in another commit because it was already there before this PR.
| import random | ||
| import logging | ||
| import warnings | ||
| import six |
There was a problem hiding this comment.
please add six to requirements and setup.py
There was a problem hiding this comment.
Thanks for catching this.
Added six and rebased against master. #64 allowed to catch 2 more issues.
hubstorage/utils.py
Outdated
| elif not isinstance(p, str): | ||
| p = str(p) | ||
| elif not isinstance(p, six.text_type): | ||
| p = six.u(str(p)) |
There was a problem hiding this comment.
this should be simply six.text_type(p)
bertinatto
force-pushed
the
py3
branch
3 times, most recently
from
d5365a8 to
cece266
Compare
tests/test_batchuploader.py
Outdated
| 'Value exceeds max encoded size of 1048576 bytes:' | ||
| ' \'{"b+\\.\\.\\.\'', | ||
| w.write, {'b'*(m/2): 'x'*(m/2)}) | ||
| w.write, {'b'*int(m/2): 'x'*int(m/2)}) |
There was a problem hiding this comment.
This is better expressed as floordiv, m // 2.
There was a problem hiding this comment.
+1
also PEP8 suggests spaces between operands, i.e. {'b' * (m // 2): 'x' * (m // 2)}
There was a problem hiding this comment.
to be fair, they are not necessary: https://www.python.org/dev/peps/pep-0008/#other-recommendations
pep8 requires one space around comparisons, boolean operators and assignments. other operators are required to have no more than one space and the same amount on both sides. also, quite a shocker for me from No section:
No:
x = x * 2 - 1
hypot2 = x * x + y * y
c = (a + b) * (a - b)
There was a problem hiding this comment.
interesting, I was taught to do this differently :) and linters usually show an error for such formatting. I don't have a strong opinion here, "Use your own judgment" can be applied.
also, quite a shocker for me from No section
haha, yeah, I thought I knew PEP8. I was wrong :)
|
Added couple nitpicks, but all in all seems nice. Do we want to enforce absolute imports with |
hubstorage/resourcetype.py
Outdated
| self.apipost(jl=self._data, is_idempotent=True) | ||
| else: | ||
| self.apipost(jl=dict((k, v) for k, v in self._data.iteritems() | ||
| self.apipost(jl=dict((k, v) for k, v in six.iteritems(self._data) |
There was a problem hiding this comment.
what about updating the python idioms: {k: v for k, v in ... }
no objections |
bertinatto
force-pushed
the
py3
branch
2 times, most recently
from
ad80a76 to
9a5c80a
Compare
May I question the use of I don't see a lot of advantages using it, since Python 3 tests will catch the use of non-absolute imports. Also, I see it's not enforced in other py3-compatible libraries. What do you think? |
No, this is why it was a question :) Ok, it would only affect intra-hubstorage import, we have control over it in tests, let's not do that. |
4 participants
Do we want to support
py3[0-2]?If we do want to support this version range we have to go over all string literals and use
six.uinstead of theuprefix literal. I'm avoiding usingunicode_literals.