Support Python 3 #36

Merged
merged 13 commits into from Apr 13, 2013

Projects

None yet

2 participants

@treyhunner
Collaborator

No description provided.

@treyhunner treyhunner and 1 other commented on an outdated diff Apr 6, 2013
model_utils/tests/tests.py
@@ -57,7 +57,7 @@ def setUp(self):
def test_unicode_content(self):
- self.assertEquals(unicode(self.post.body), self.full_text)
+ self.assertEquals(str(self.post.body), self.full_text)
@treyhunner
treyhunner Apr 6, 2013 Collaborator

The fact that this test is called test_unicode_content makes me think changing unicode to str might not be a good idea here. However, I don't see how this test has anything to do with unicode since the unicode string used contains no characters outside of the ASCII/Latin 1 character set.

@carljm
carljm Apr 8, 2013 Owner

I agree that this test isn't actually testing anything in particular about unicode, but using str here is still wrong. For Python 2/3 cross-compatible code, any use of str is pretty much always wrong, because str means something entirely different in Python 2 vs Python 3; in 2 it means a byte string and in 3 it means a unicode string.

@carljm carljm commented on an outdated diff Apr 8, 2013
model_utils/fields.py
@@ -150,6 +150,9 @@ def _get_has_more(self):
return self.excerpt.strip() != self.content.strip()
has_more = property(_get_has_more)
+ def __str__(self):
@carljm
carljm Apr 8, 2013 Owner

I think instead the __unicode__ method should be renamed to __str__ (and should return text; that is a unicode instance on Python 2 and a str instance on Python 3), and the python_2_unicode_compatible decorator should be applied; see https://docs.djangoproject.com/en/dev/topics/python3/#str-and-unicode-methods

@carljm
Owner
carljm commented Apr 8, 2013

General comment on this: might be worth a read through https://docs.djangoproject.com/en/dev/topics/python3/ because that describes the approach I think we should take with this port. That includes using from __future__ import unicode_literals in every file and making use of django.utils.six as needed (it contains wrappers that can replace ambiguous built-ins like str, which mean different things on Py 2 vs 3).

Using django.utils.six and things like python_2_unicode_compatible implies dropping support for Django 1.2 and 1.3, but I'm fine with that. Whatever version of model-utils gets Python 3 support can be v2.0 and drop those older Django versions.

@treyhunner
Collaborator

It probably wouldn't be too difficult to keep Django 1.3 support with some clever try-excepts and such. I don't use Django 1.3 for any websites anymore so I'd prefer to drop support though.

I'll try updating this code to use django.utils.six and drop 1.2/1.3 support.

@carljm
Owner
carljm commented Apr 9, 2013

The issue with keeping Django 1.3 support is that django.utils.six and python_2_unicode_compatible were only backported to 1.4, so if we make use of those utilities to cover up 2/3 differences, we can't support anything prior to 1.4.

@carljm carljm commented on an outdated diff Apr 9, 2013
model_utils/choices.py
@@ -72,4 +76,4 @@ def __getitem__(self, index):
def __repr__(self):
return '%s(%s)' % (self.__class__.__name__,
- ', '.join(("%s" % str(i) for i in self._full)))
+ ', '.join(("%s" % smart_text(i) for i in self._full)))
@carljm
carljm Apr 9, 2013 Owner

I think actually str(i) here should be replaced with repr(i)

@carljm
carljm Apr 9, 2013 Owner

(This would have been more correct even to begin with, so its not really part of the port, the port just highlights it. smart_text is definitely a change in behavior here, since str on Py2 is bytes, not text.)

@carljm carljm commented on an outdated diff Apr 9, 2013
model_utils/tests/tests.py
@@ -57,7 +58,7 @@ def setUp(self):
def test_unicode_content(self):
- self.assertEquals(unicode(self.post.body), self.full_text)
+ self.assertEquals(smart_text(self.post.body), self.full_text)
@carljm
carljm Apr 9, 2013 Owner

For strict equivalency of behavior, we should replace unicode with django.utils.six.text_type. django.utils.encoding.smart_text is probably ok but introduces a bunch of extra behavior around lazy objects that we really don't need here.

@carljm carljm commented on an outdated diff Apr 9, 2013
model_utils/tests/tests.py
self.post.body.content = new_text
self.post.save()
- self.assertEquals(unicode(self.post.body), new_text)
+ self.assertEquals(smart_text(self.post.body), new_text)
@carljm
carljm Apr 9, 2013 Owner

Again, text_type rather than smart_text

@treyhunner
Collaborator

Thanks for the advice on these changes. I cleaned up the commits to take your feedback into account.

The repr tests broke for Python 2 when the string literals changed to unicode. Fix 2f52774 works and continues to use exact string equality for the repr comparison. Fix cc19661 instead relies on tuple and function call repr equality. I prefer the second fix because it doesn't rely on string equality.

@carljm
Owner
carljm commented Apr 9, 2013

I agree, the second fix is nicer. I was wondering about the binary_type(...), but it looks like you just fixed that!

@treyhunner
Collaborator

Great. I just squashed the last three commits. It seems like it may be ready for a merge now.

There are probably some changes that can be made after merge now that only Django 1.4+ is supported. The only one I'm aware of currently is the datetime.datetime/django.utils.timezone use.

These changes were a little more guess-and-check than usual because I'm not yet very familiar with six, django.utils.encoding, or Python 3.

@carljm
Owner
carljm commented Apr 9, 2013

Super; let's see what the Travis build has to say about it :-)

Also, don't forget to update CHANGES.rst with the news of Python 3 support and the dropped Django versions. (Also may need an update for the StatusField fix you just merged.)

@carljm
Owner
carljm commented Apr 9, 2013

Oh, and running the tests locally I notice that under Python 3 there are a bunch of deprecation warnings due to use of self.assertEquals and self.assert_ (should now be self.assertEqual and self.assertTrue, respectively). S'pose now is the right time to clean those up, too.

@treyhunner
Collaborator

I agree. The new(er) TestCase methods should be used before merging.

@carljm
Owner
carljm commented Apr 10, 2013

I also noticed that the test coverage after a full tox run is no longer 100% in this branch. Part of that is due to the fallback for django.utils.timezone.now, which can just be removed. It also looks like the "no_check_for_status" case is no longer tested in StatusField? Not sure yet why that is.

@carljm
Owner
carljm commented Apr 10, 2013

Looks like the coverage issue on a tox test run with StatusField is in master too - but strangely coveralls still shows 100% coverage on master.

@treyhunner
Collaborator

I'm not sure what you mean about no_check_for_status not being tested. The South case isn't run against Python 3 at all currently because the latest South doesn't support Python 3 yet. I'm not sure if that's what you mean.

https://coveralls.io/builds/15856/source?filename=model_utils%2Ffields.py

@carljm
Owner
carljm commented Apr 10, 2013

I just pushed a fix to master for the test coverage issue with StatusField no_check_for_status. Not sure why it didn't show up until your recent fix. (It's not an issue with this branch, so I confused things by commenting about it here). It only showed up as missing coverage in tox runs, not in coveralls, because coveralls doesn't check branch coverage. Tox was showing that the "if" statement on line 58 of fields.py was never covering the False branch (i.e. do nothing). Fixed now, sorry for the confusion.

@treyhunner
Collaborator

Should the supported Python versions be noted in the setup.py file (like in requests)?

What about the minimum supported Django version? Should that simply be left stated in the README file?

@carljm
Owner
carljm commented Apr 12, 2013

Yeah, adding the Python version trove classifiers in setup.py would be great.

As for minimum Django version, really we should probably add an install_requires dependency on Django>=1.4.2 (if we're doing it in this branch).

@carljm
Owner
carljm commented Apr 12, 2013

(I think the trove classifiers also get us a "green" rating on some of the "Python 3 compatibility" charts without manual intervention, which is nice.)

treyhunner added some commits Apr 6, 2013
@treyhunner treyhunner Fix iterator/list problems for Python 3 support 81c7e40
@treyhunner treyhunner Fix str/unicode problems in Python 3
Changes:
- Use unicode_literals from the future for Python 2.6.5+
- Use six's text_type for proper unicode use in Python 2/3
4f2673e
@treyhunner
Collaborator

Seem merge-ready to you?

I can't think of anything else that's needed.

@carljm carljm commented on an outdated diff Apr 12, 2013
@@ -29,7 +29,8 @@ your ``INSTALLED_APPS`` setting.
Dependencies
------------
-``django-model-utils`` is tested with `Django`_ 1.2 and later on Python 2.6 and 2.7.
+``django-model-utils`` supports `Django`_ 1.4.2 and later on Python 2.6, 2.7,
+and 3.3.
@carljm
carljm Apr 12, 2013 Owner

3.2 is missing from this list?

@carljm carljm commented on an outdated diff Apr 12, 2013
model_utils/fields.py
from django.db import models
from django.conf import settings
-
-
-try:
- from django.utils.timezone import now as now
-except ImportError:
- now = datetime.now
+from django.utils.encoding import python_2_unicode_compatible
+from django.utils.timezone import now as now
@carljm
carljm Apr 12, 2013 Owner

The as now in this import is totally redundant. Not sure how I missed that when I merged the original timezone-support PR.

@carljm carljm commented on an outdated diff Apr 12, 2013
model_utils/models.py
from django.db import models
from django.utils.translation import ugettext_lazy as _
from django.db.models.fields import FieldDoesNotExist
from django.core.exceptions import ImproperlyConfigured
+from django.utils.timezone import now as now
@carljm
carljm Apr 12, 2013 Owner

as now is also redundant here

@carljm carljm commented on an outdated diff Apr 12, 2013
model_utils/tests/tests.py
@@ -1,4 +1,4 @@
-from __future__ import with_statement
+from __future__ import unicode_literals, with_statement
@carljm
carljm Apr 12, 2013 Owner

We can remove the with_statement future import now that the minimum supported Python is 2.6.

@carljm
Owner
carljm commented Apr 12, 2013

Read through it again and noticed a few minor things; once those are corrected it looks merge-ready to me!

@treyhunner
Collaborator

Fixed those and also added 3.2 to tox file.

I'll rebase and merge after tox and Travis confirm all is good.

@carljm
Owner
carljm commented Apr 12, 2013

lgtm!

@carljm
Owner
carljm commented Apr 12, 2013

Oh wait, I take that back :-) We've still got the deprecation warnings in Python 3 due to use of assertEquals and assert_; should be assertEqual and assertTrue instead.

@treyhunner
Collaborator

Yup that's all over my tox output. Fixing and then merging.

@treyhunner treyhunner merged commit 02cc04b into master Apr 13, 2013

1 check passed

default The Travis build passed
Details
@treyhunner treyhunner deleted the python3 branch Apr 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment