Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#14910 closed (fixed)

I18N locale per app resolving - documentation incorrect and bug

Reported by: Klaas van Schelven Owned by: nobody
Component: Internationalization Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,
According to
http://docs.djangoproject.com/en/1.2/howto/i18n/#using-internationalization-in-your-own-projects
and
http://docs.djangoproject.com/en/1.2/topics/i18n/deployment/#how-django-discovers-translations

At runtime, Django looks for translations by following this algorithm:

  • First, it looks for a locale directory in the application directory of the view that’s being called. If it finds a translation for the selected language, the translation will be installed.
  • Next, it looks for a locale directory in the project directory. If it finds a translation, the translation will be installed.
  • Finally, it checks the Django-provided base translation in django/conf/locale.

However, this has not been my experience. I also do not see anything
in the django source code or tests that responds to the above.
In django/utils/translation/trans_real.py I see the following:

def translation(language): 
     [......] 
        for appname in settings.INSTALLED_APPS: 
            apppath = os.path.join(get_apppath(appname), 'locale') 
            if os.path.isdir(apppath): 
                res = _merge(apppath)

_merge (and deeper down "merge") is defined as a dictionary update in
the same file.

I see two problems here:

  1. the documented behavior is not implemented
  2. the implemented behavior has the opposite order-of-preference for apps as the rest of Django. (Normally, the higher up in your INSTALLED_APPS you put an app, the more likely it's templates, etc. will be called. Here we're using a dict update, so the lowest app has preference)

Also: I think the documented behavior is not desirable. Changing
translations per-view makes for hard to debug once-in-a-while bugs.
This means the bug '2' is still a problem. I suggest either reversing
the loop

158         for appname in settings.INSTALLED_APPS: 

or making the update of a 'merge' non-destructive.

Reported before on Django-Developers, but I got no response:
http://groups.google.com/group/django-developers/browse_thread/thread/6c098e16da4c49ae

Attachments (4)

trans_real.patch (2.8 KB ) - added by Klaas van Schelven 13 years ago.
docs.patch (2.0 KB ) - added by Klaas van Schelven 13 years ago.
New docs (half the problem was incorrect docs)
with_test.patch (5.2 KB ) - added by Klaas van Schelven 13 years ago.
Patch with test
docs.current_situation.patch (2.1 KB ) - added by Klaas van Schelven 13 years ago.
Fixed docs for the current situation. Point 1. of the original posting.

Download all attachments as: .zip

Change History (16)

comment:1 by Klaas van Schelven, 13 years ago

Django's default template loader is documented here:

http://docs.djangoproject.com/en/dev/ref/settings/?from=olddocs#template-loaders

('django.template.loaders.filesystem.Loader',
 'django.template.loaders.app_directories.Loader')

Keeping with that order I propose the following changes:

  • First look at explicit locale paths, then the project path, then the app paths.
  • Make updates of the locale dict non-destructive

I'll add a patch once my tests are done running.

by Klaas van Schelven, 13 years ago

Attachment: trans_real.patch added

comment:2 by Klaas van Schelven, 13 years ago

Has patch: set

Having looked at the original tests the most desirable order appears to be:

  1. Project path locale
  2. Apps (in order)
  3. Locale-path
  4. Django's own translations

The attached patch is a reflection of

  1. The order mentioned above
  2. Non-destructive dictionary updates
  3. I had to move the monkey-patching code into the _merge function, because the first actually found locale is no longer known in advance.

by Klaas van Schelven, 13 years ago

Attachment: docs.patch added

New docs (half the problem was incorrect docs)

comment:3 by Jannis Leidel, 13 years ago

Needs tests: set

Hm, I wonder if this is related to r12447.

There are definitely tests that make sure this works, so I'd appreciate it if you provide testcases for this issue.

comment:4 by Jannis Leidel, 13 years ago

milestone: 1.3

by Klaas van Schelven, 13 years ago

Attachment: with_test.patch added

Patch with test

in reply to:  4 comment:5 by Klaas van Schelven, 13 years ago

Replying to jezdez:

It's related to r12447 in a minor way: that revision provides a nice testbed, though if you read the tests you'll see that they don't match with the documentation.

I've also added a test in the latest patch. (I'm not sure why the quickview shows 2 files as /dev/null, but I'm sure you'll figure it out).

in reply to:  description ; comment:6 by Ramiro Morales, 13 years ago

Component: UncategorizedInternationalization
Triage Stage: UnreviewedAccepted

Replying to vanschelven:

Hi,
According to
http://docs.djangoproject.com/en/1.2/howto/i18n/#using-internationalization-in-your-own-projects
and
http://docs.djangoproject.com/en/1.2/topics/i18n/deployment/#how-django-discovers-translations

At runtime, Django looks for translations by following this algorithm:

  • First, it looks for a locale directory in the application directory of the view that’s being called. If it finds a translation for the selected language, the translation will be installed.
  • Next, it looks for a locale directory in the project directory. If it finds a translation, the translation will be installed.
  • Finally, it checks the Django-provided base translation in django/conf/locale.

However, this has not been my experience. I also do not see anything
in the django source code or tests that responds to the above.

From what I've found doing some code archeology, this was the original behavior, note the First, it looks for a locale directory in the application directory of the view that’s being called. part. Note there is no mention of iterating over apps that appear in INSTALLED_APPS.

A key part of the scheme (IMHO missed by e.g. #11384) was this paragraph located a few paragraphs below: Application message files are a bit complicated to discover – they need the LocaleMiddleware. If you don’t use the middleware, only the Django message files and project message files will be installed and available at runtime.

It was LocaleMiddleware's task to activate translations from the app containing the current view: http://code.djangoproject.com/browser/django/branches/i18n/django/middleware/locale.py?rev=963#L9

But this was changed in r964, documentation wasn't updated accordingly and have remained untouched and unchallenged for five years. So much e.g. for my i18n docs refactoring :(

I haven thought about the OP proposals yet, just wanted to get these notes written down. IMHO this ticket should deal with what OP call item 1. Item 2 should be reported and dealt in another ticket, one issue per ticket/commit.

in reply to:  6 comment:7 by Klaas van Schelven, 13 years ago

Replying to ramiro:

I haven thought about the OP proposals yet, just wanted to get these notes written down. IMHO this ticket should deal with what OP call item 1. Item 2 should be reported and dealt in another ticket, one issue per ticket/commit.

In that case I would suggest to adapt the documentation to reflect the reality of the code:

  1. This won't break any existing code
  2. It makes more sense to have static translations than dynamic ones. Behavior will be more reliable, debugging and testing will be easier.

by Klaas van Schelven, 13 years ago

Fixed docs for the current situation. Point 1. of the original posting.

comment:8 by Klaas van Schelven, 13 years ago

This ticket is about "point 1" above from now on.

Point 2 is moved out here: #14924

comment:9 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

(In [14998]) Fixed #14910 -- Clarified the order of precedence of loading translation catalogues. Thanks, vanschelven.

comment:11 by Jannis Leidel, 13 years ago

(In [15002]) [1.2.X] Fixed #14910 -- Clarified the order of precedence of loading translation catalogues. Thanks, vanschelven.

Backport from trunk (r14998).

comment:12 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

Note: See TracTickets for help on using tickets.
Back to Top