Skip to content
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

Python 3 support to convert Token objects to str #2183

Closed
wants to merge 1 commit into from
Closed

Python 3 support to convert Token objects to str #2183

wants to merge 1 commit into from

Conversation

cript0nauta
Copy link
Contributor

__unicode__ method doesn't work on Python 3

__unicode__ method doesn't work on Python 3
@tomchristie
Copy link
Member

Looks like there's some accidental extra whitespace that needs to be removed.

@tomchristie
Copy link
Member

Also worth finding out what the standard way for doing str and unicode is and ensuring its py3 compatible. Simply having both if these returning the same thing surely can't be correct.

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 2, 2014

@tomchristie six provides wrappers for that:

  • six.text_type for unicode in Python 2 and str in Python 3.
  • six.binary_type for str in Python 2 and bytes in Python 3.

@xordoquy
Copy link
Collaborator

xordoquy commented Dec 2, 2014

As for Django model, there's python_2_unicode_compatible decorator (https://docs.djangoproject.com/en/dev/topics/python3/#str-and-unicode-methods). I think I proposed the change a while ago but was rejected, not sure why.

@tomchristie
Copy link
Member

not sure why

Me too, I expect.

@maryokhin
Copy link
Contributor

btw, if we would raise supported Django version from 1.4.16 to 1.4.2, then we could remove https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/compat.py#L328

@jpadilla
Copy link
Member

jpadilla commented Dec 2, 2014

@maryokhin nice catch, I think that's a sensible change we can do.

@tomchristie
Copy link
Member

Do you mean from 1.4.11 (current) to 1.4.16?

@jpadilla
Copy link
Member

jpadilla commented Dec 2, 2014

@tomchristie @maryokhin I think we can actually just jump to latest 1.4.* which includes python_2_unicode_compatible to forward compatibility with Django 1.5, allowing us to remove that from compat.py.

Update: Maybe 1.4.11 does include python_2_unicode_compatible. Checking...
Update: Just confirmed 1.4.11 does in fact have python_2_unicode_compatible.

@maryokhin
Copy link
Contributor

@tomchristie I said 1.4.16 because I always base my opinion on which version a project supports depending on which version the project is actually tested against. Although in the docs it says 1.4.11, tox runs against 1.4.16, so I presumed that it is the supported version.

@jpadilla I just realized that updating from 1.4.16 to 1.4.2 didn't make sense at all. Treated .2 as .20, duh, that was stupid.

I can do the compat removal thing and update the docs with new minor versions while I'm at it.

@jpadilla
Copy link
Member

jpadilla commented Dec 2, 2014

@maryokhin sounds good.

@sh4r3m4n I think going with python_2_unicode_compatible would be the right choice.

@tomchristie
Copy link
Member

on which version the project is actually tested against

Yup, fair enough. Not obvious if we should bump the tested minor versions down, or just leave the disparity.

@maryokhin
Copy link
Contributor

Working on this now.

@tomchristie What's the final decision on versions? Lower tox or bump up supported version?

@sh4r3m4n Are you woking on the decorator thing or should I just add it along with my stuff? Either way is fine, just want an update on the status of things.

@tomchristie
Copy link
Member

@maryokhin I'd suggest we change tox to test against:

  • Lowest supported versions of 1.4, 1.5 (1.4.11, 1.5.5)
  • Highest supported versions of 1.6, 1.7 (1.6.9, 1.7.2)

maryokhin added a commit to maryokhin/django-rest-framework that referenced this pull request Dec 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants