Pull requests

#6 Declined
Repository
amarandon amarandon
Branch
default
Repository
carljm carljm
Branch
default

Changes for issue #23

Author
  1. Alex Marandon
Reviewers
Description

Hi Carl,

This pull request is based on the patch suggested by Sylvain Fourmanoit. I've added tests.

Cheers, Alex

Comments (8)

  1. Carl Meyer repo owner

    Hi Alex, thanks for the contribution.

    This looks pretty reasonable. There are a couple typos in the docs ("important to understand tha*n* just following a localized" and three instances of "LOCALURL" where it should be "LOCALEURL"), but I can fix those when I merge.

    The more important problem is that when I run "tox", I'm seeing 2 test failures under Django trunk with this pull request, where everything passes with current localeurl tip. I'm not sure if this is actually a bug in your code, or that your added tests are exercising something that is already broken in localeurl with Django trunk but not adequately tested. Either way I'm afraid I don't have time to look into it right now, and I can't merge this with failing tests, so if you're able to look into it that would be a big help.

  2. Alex Marandon author

    The problem seems to be caused by the way the test suite uses reload(). At least this the only plausible explanation I've got so far. I've tried to remove all calls to reload() from my own test code but the problem persists. As it's beyond the scope of my patch to fix the test suite so that it allows using Django test client, my latest commit removes tests for change_locale. This isn't making things worse since there's no test coverage of change_locale in current localeurl tip.

    1. Carl Meyer repo owner

      Turns out that the reload of urlresolvers was the one causing the problem, and that reload wasn't even necessary! With that removed, all tests pass. Thanks for your work on this!

  3. Carl Meyer repo owner

    Ok, thanks for looking into it. I'd really rather merge this with the tests, so I'll look into first removing all use of reload in the test suite, and see if that allows this to merge with passing tests.

  4. Alex Marandon author

    Thanks. If this is a burden though, I'd be happy to give up on the changes to change_locale. To tell you the truth we don't use it in our project. I only included it because it was part of the patch suggested by Sylvain.

  5. Carl Meyer repo owner

    Hmm, within the framework of what django-localeurl provides, I don't think it would make much sense to have the middleware check the session var, but not set the session var in the obvious place to do so. I want to get rid of those uses of reload anyway, they're ugly.