Pull requests

#14 Open
Repository
yds19872712 yds19872712
Branch
default
Repository
carljm carljm
Branch
default

Better support for newer Django and Python 3

Bitbucket cannot automatically merge this request.

The commits that make up this pull request have been removed.

Bitbucket cannot automatically merge this request due to conflicts.

Review the conflicts on the Overview tab. You can then either decline the request or merge it manually on your local system using the following commands:

hg update default
hg pull -r default https://bitbucket.org/yds19872712/django-localeurl
hg merge 6ffa968b657d
hg commit -m 'Merged in yds19872712/django-localeurl (pull request #14)'
Author
  1. Dmytro Yurchenko
Reviewers
Description
No description
  • Learn about pull requests

Comments (11)

  1. Dmytro Yurchenko author

    Hi, I think I've changed 'django_language' to '_language' too early. A version check has to be added like this:

    MAJOR, MINOR = django.VERSION[:2]
    if MAJOR >= 1 and MINOR >= 7:
        LANGUAGE_SESSION_KEY = '_language'
    else:
        LANGUAGE_SESSION_KEY = 'django_language'
    
      1. Dmytro Yurchenko author

        As far as I've understood: the language cookie and the language variable in the session are different things. And in the session all underscore prefixed variables are reserved by Django. That is the reason why they changed django_language to _language, to be more consistent. Current dev version of Django already dropped compatibility support for 'django_language'. https://code.djangoproject.com/ticket/5789

        1. Carl Meyer repo owner

          You're right of course; forgot that there is both a cookie and a session var.

          Is this change actually necessary for LocaleURLMiddleware to function with newer Django, or is it simply a theoretical matter of maintaining consistency with the behavior of Django's LocaleMiddleware? Unless I remember wrong (I'm afraid I am a bit rusty on localeurl as I haven't used it recently), LocaleURLMiddleware is intended as a replacement for LocaleMiddleware, so it shouldn't really matter if they use a different session var (except in that it would cause users to lose their language setting if a site switches from using LocaleURLMiddleware to LocaleMiddleware or vice versa, which I guess is a small issue).

          If we wanted to maintain full consistency with LocaleMiddleware, I think we would actually duplicate the soft-upgrade behavior implemented in https://github.com/django/django/commit/0d0f4f020afe516f23fd2305f13ff0a6a539b344#diff-06e503a83071c41ac1c991792bf09c56R15 if we detect Django version 1.7, and use "django_language" for <=1.6 and "_language" for >=1.8. But in practice I think it's good enough if we just implement the simple hard switch like you showed. Do you mind adding that to the pull request?

          1. Dmytro Yurchenko author

            Well, my main goal was to get rid of infinite loop of redirection in python 3... b'b'b'b'b'b'b'''''''''... which was caused by encode, which was added to avoid u'u'u'u'u''''' loop in python 2. six.u() replaced ordinary u'' prefix cause 3.0-3.2 versions are still default python3 on many systems. I think to duplicate the soft-upgrade behavior is the best way to go, cause if it started to be more LocaleMiddleware compatible by naming variable django_language, then it has to be fully compatible or we better rename it to django_localeurl_language. I will gladly rethink and redo my pull request closer to the weekend.

  2. Dmytro Yurchenko author

    Since the docs say "The localeurl application requires Django 1.3 or higher" it is time to remove iri_to_uri from

    # @@@ iri_to_uri for Django 1.0; 1.1+ do it in HttpResp...Redirect
    return redirect_class(iri_to_uri(locale_url))
    

    I've rechecked and confirm that since Django 1.1.4 HttpResponseRedirect (both of them) apply iri_to_uri by themselves.

  3. Carl Meyer repo owner

    Unfortunately this PR currently breaks several tests on Python 2, and isn't sufficient to make the tests pass on Python 3. I think the changes needed may not be major, but I don't have time to dig into it further right away. If you are able to update it such that the tests pass, I will gladly merge it.

    Easiest way to run the tests on all supported environments is to pip install tox and then run tox (this requires that you have python2.6, python2.7, python3.3, and python3.4 on your shell PATH). Here is the diff necessary to add Python 3.3 and 3.4 to the tox.ini file: https://gist.github.com/carljm/11015570