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

Fix for issue #10 (AttributeError: NoneType for lang) #12

Merged
merged 5 commits into from
Aug 10, 2017

Conversation

jm9e
Copy link
Contributor

@jm9e jm9e commented Aug 8, 2017

In case translation.deactivate_all() from django.utils has been called, translation.get_language() returns None. This caused an error in the i18n_l10n_prefix method.

This pull request contains a test which examines exactly this case, testing the error case and the correctness of the returned prefix for that case. It also contains a simple fix for this problem.

@zsoldosp
Copy link
Owner

zsoldosp commented Aug 8, 2017

@jm9e looks good to me too. Could you please bump the version number to 0.1.5 & add the release notes (or should we do that after merging, @kairichard)?

minor: IMO it would be more correct to use return '.'.join(p for p in parts if p) (which we didn't do originally 'coz it didn't occur to us that lang/locale can be None ☹️), but the current approach does fix a bug, which is more important than the exact cache key used, so I wouldn't block the release for this

@kairichard
Copy link
Contributor

@jm9e @zsoldosp Let's add release notes to this branch and then merge it. We did that with the previous ones too.

@jm9e
Copy link
Contributor Author

jm9e commented Aug 9, 2017

@zsoldosp @kairichard Thanks for your feedback. I updated the version and added release notes as requested.

@kairichard kairichard added the LGTM label Aug 9, 2017
@kairichard kairichard merged commit 63de1e6 into zsoldosp:master Aug 10, 2017
@zsoldosp
Copy link
Owner

@jm9e thanks for the fix, and the new release is up on pypi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants