Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Python3 support #22

Closed
wants to merge 11 commits into from
Closed

Python3 support #22

wants to merge 11 commits into from

Conversation

tricoder42
Copy link

Added Python2/3 compatability layer through six. Tests pass on python 2.7, 3.2, 3.3.

@@ -401,7 +402,7 @@ def elements(self):
"""
for element, count in self._data():
if count:
for _ in xrange(0, count):
for _ in range(0, count):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But range in Python 2.X is not the same as range in Python 3.X, isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I wasn't precise enough :) There's fix in a0d966b

@honzajavorek
Copy link
Collaborator

Seems like a good work, thanks! Usage of six is (optically) ugly though, and it is a new requirement. I like the idea presented here. However, I don't use Python3 and I never ported anything to Python3, so I can't comment it much.

One serious issue: I am afraid I can't maintain the project in multiple versions. I'd appreciate any help with all this. It is a kind of challenge for me, I can't just learn it in one day and then be ready to fix bugs and run tests on such "dual" codebase.

@tricoder42
Copy link
Author

The article, you've mentioned, implements similar constructs as six does. with_metaclass, iter_* functions, PY3 constant etc. I do understand it's another requirenment. However, since so many huge projects use six as well, I don't consider it as a problem.

The good think about six is, you mantain only one codebase. Sure, you have to test new code agains multiple versions, but I'm willing to help you with that since I'm going to use this library.

@@ -366,7 +370,7 @@ def __init__(self, *args, **kwargs):
super(Counter, self).__init__(*args, **kwargs)

def _pickle(self, data):
return unicode(int(data))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what is purpose of this line. Test suite passes even with identity function (return data). Why you convert data to int and then to unicode? I'm asking because my "fix" isn't precise either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is type check (int) and serialization (unicode). In this implementation of Counter, data has to be integer only and then they are serialized for Redis just by calling unicode. Maybe it involves some redundancy, but it is improving readability and consistency. I agree coercing to int is stupid "validation", but it did the job quite well so far.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure. If int is called as type check only, it would work. There's no unicode in Py3 and "it works" even without it. I'll try to fix it so it reflects original behavior.

Edit: Added d873e1a

@honzajavorek
Copy link
Collaborator

If you agree to take care about the PY3 version and compatibility testing, I am open to accept your changes regardless your tools (six). I can't do better/differently myself anyway and something working is definitely better than nothing, ugly or not :-) This way I won't even have to install multiple Python versions, TravisCI should stop me before releasing versions with broken PY3 tests to PyPI.

@tricoder42
Copy link
Author

Sure, I'm in. As I said, I'm going to use Py3 version, from your official repository or forked one. Who uses Python 2.x these days anyway? :)

@honzajavorek
Copy link
Collaborator

Me! :-D

@tricoder42
Copy link
Author

_pickle method should be consistent now. six.b(str(data)) is ugly, but six.b can't take int as argument, so I need to convert it to str/unicode and then to str/bytes.

@honzajavorek
Copy link
Collaborator

I think I should write tests, which could possibly break your PY3 port and if they won't, I can merge it :-) That's probably the best idea for now.

@tricoder42
Copy link
Author

There's no rush. I still haven't used it in production. That would be definitely good source for bugs and tests.

@honzajavorek
Copy link
Collaborator

Actually, there is. I got some e-mails about PY3 support from other people :)

@puredevotion
Copy link

I'm willing to use this with PY3 as well.

I get basestring errors, (NameError: global name 'basestring' is not defined) the following proposes a solution (which has more or less been mentioned above): oxplot/fysom#1

@sc68cal
Copy link

sc68cal commented Feb 23, 2014

+1 for basestring errors

127.0.0.1:6379> LPUSH test foo
(integer) 1
>>> from redis_collections import List
>>> a = List(key='test')
>>> a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/scollins/src/vote_rings/.venv/lib/python3.3/site-packages/redis_collections/base.py", line 296, in __repr__
    data = self._repr_data(self._data())
  File "/Users/scollins/src/vote_rings/.venv/lib/python3.3/site-packages/redis_collections/lists.py", line 361, in _repr_data
    return repr(list(data))
  File "/Users/scollins/src/vote_rings/.venv/lib/python3.3/site-packages/redis_collections/lists.py", line 74, in <genexpr>
    return (self._unpickle(v) for v in values)
  File "/Users/scollins/src/vote_rings/.venv/lib/python3.3/site-packages/redis_collections/base.py", line 225, in _unpickle
    if not isinstance(string, basestring):
NameError: global name 'basestring' is not defined

@thaeli
Copy link

thaeli commented Nov 4, 2014

+1 for this - Same error here, it makes this module unusable for me in its current state.

@honzajavorek
Copy link
Collaborator

I don't have time (motivation) currently to maintain redis-collections much (not even for PR reviews, which is pretty sad) and I'm looking for a maintainer. It wouldn't be much hassle as there are just minor bugs with existing PRs and the only major feature is Python 3 support and it also has this PR in progress. If anyone feels like taking over this library, it would be much appreciated.

@bbayles bbayles mentioned this pull request Jul 30, 2016
@bbayles
Copy link
Collaborator

bbayles commented Jul 30, 2016

@tricoder42, I'm going to close this PR in favor of #44, which similarly moves toward supporting Python 3. Thanks!

@bbayles bbayles closed this Jul 30, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants