Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#14924 closed (fixed)

I18N looks for translations in the reverse order of the apps

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

Description

I18N should look for translation strings in the order of apps as given in INSTALLED_APPS. Translations in apps that appear earlier in that list should be used rather than translations later in that list in case there are conflicts.

As described in #14910 as "point 2". I'm reproducing the important parts here:

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')

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

  1. Project path locale
  2. Apps (in correct 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.

Attachments (8)

with_test.patch (5.3 KB) - added by vanschelven 3 years ago.
Patch with test.
docs.patch (2.0 KB) - added by vanschelven 3 years ago.
Docs patch for the current documentation
docs.after.14910.patch (1.0 KB) - added by vanschelven 3 years ago.
The docs patch to apply if #14910 is fixed
with_tests_2.patch (5.3 KB) - added by vanschelven 3 years ago.
Patch file syntax correct
django.mo (89 bytes) - added by vanschelven 3 years ago.
.mo that goes with the .po (part of the patch)
14924.1.diff (13.7 KB) - added by ramiro 3 years ago.
Patch with modification of loading precedence as proposed by vanschelven + tests + docs mods. Remeber to regenerate the .mo files correspondig to the touche .po files
14924.2.diff (20.5 KB) - added by ramiro 3 years ago.
New version of the patch, also includes fix for #10765. start deprecation path for loading of translations from the 'project' directory.. remeber to generate the .mo files under the tests/ subdir for the three modified .po files
14924.3.diff (26.7 KB) - added by ramiro 3 years ago.
New patch: Removes count trickery in PendingDeprecationWarning code, expands documentation, adds release note blurbs.

Download all attachments as: .zip

Change History (19)

Changed 3 years ago by vanschelven

Patch with test.

Changed 3 years ago by vanschelven

Docs patch for the current documentation

Changed 3 years ago by vanschelven

The docs patch to apply if #14910 is fixed

Changed 3 years ago by vanschelven

Patch file syntax correct

Changed 3 years ago by vanschelven

.mo that goes with the .po (part of the patch)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 in reply to: ↑ description Changed 3 years ago by vanschelven

I'd like to have some discussion on my own comment below:

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

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

I've got the above patch running in some production code now, and I'm feeling this is not the most logical order. (The order was originally established by looking at the existing tests and code, and matching that order as closely as possible while making sure the apps are evaluated in the correct, not reverse, order).

In my mind, locale path should be the first to be evaluated. The reason someone would put up a specific path for locales pretty much has to be to override the existing behavior (which would be found in project and apps).

Input (discussion) on this is much appreciated. I'll gladly put in a new patch once a decision is reached. If this is a separate issue, I'm also fine on opening a separate ticket. (it could be argued that we can move forward on the original bug report as a separate issue more easily, since it's "obviously broken").

comment:3 Changed 3 years ago by ramiro

  • Component changed from Uncategorized to Internationalization

Changed 3 years ago by ramiro

Patch with modification of loading precedence as proposed by vanschelven + tests + docs mods. Remeber to regenerate the .mo files correspondig to the touche .po files

comment:4 Changed 3 years ago by ramiro

  • Has patch set

The 14924.1.diff patch implements the modifcations discussed here and in the mailing list thread. Namely, to modify the precedence of loading of translations from compiled message files from the current situation (from higher to lesser):

  • The translation found in locale/ subdir of the the project dir.
  • The translations found in the apps listed in settings.INSTALLED_APPS with the latter ones having higher precedence.
  • The translations found in the paths listed in settings.LOCALE_PATHS with the latter ones having higher precedence.
  • The translations shipped with Django.

To (also from higher to lesser precedence):

  • The translations found in the paths listed in settings.LOCALE_PATHS with the ones located first having higher precedence.
  • The translation found in locale/ subdir of the the project dir.
  • The translations found in the apps listed in settings.INSTALLED_APPS with the ones located first having higher precedence.
  • The translations shipped with Django.

As proposed by vanschelven.

Tests and docs were modified accordingly. Some of these tests/docs mods are tweaks that can be extracted and applied even if this ticket isn't accepted or committing of the patch is deferred.

Changed 3 years ago by ramiro

New version of the patch, also includes fix for #10765. start deprecation path for loading of translations from the 'project' directory.. remeber to generate the .mo files under the tests/ subdir for the three modified .po files

comment:5 Changed 3 years ago by ramiro

14924.2.diff (ignore the #10765 reference in the ticket description , should be #5494) contains a further evolution to the patch that:

  • Extends the changes to include building of JavaScript catalog in the javascript_catalog() view making it more consistent with the one used for Python/templates code translations (paths listed in LOCALE_PATHS is taken in account.)
  • As per discussion with Jannis and Claude Paroz, starts the deprecation path for inclusion of translations located under the project path. The LOCALE_PATH setting can be used for the same task by listing the filesystem path to the project level translations. Reationale for this:
    • The project path has always been an elusive concept (it is actually the directory containing the settings) and there has been a shift in other parts of the framework from using it as a reference for location of assets at runtime.
    • It fails when the settings module is a directory (#10765)
    • The project_dir/locale/ subdir can generate spurious error messages when the project directory is included in the python path (default behavior of manage.py runserver), e.g.: /usr/lib/python2.6/gettext.py:49: ImportWarning: Not importing directory '/path/to/project/dir/locale': missing __init__.py. See also http://groups.google.com/group/django-users/browse_thread/thread/e8bb9089d9e5be60?pli=1

comment:6 Changed 3 years ago by jezdez

  • milestone set to 1.3
  • Triage Stage changed from Accepted to Ready for checkin

Changed 3 years ago by ramiro

New patch: Removes count trickery in PendingDeprecationWarning code, expands documentation, adds release note blurbs.

comment:7 Changed 3 years ago by ramiro

  • Resolution set to fixed
  • Status changed from new to closed

In [15441]:

Fixed #5494, #10765, #14924 -- Modified the order in which translations are read when composing the final translation to offer at runtime.

This is slightly backward-incompatible (could result in changed final translations for literals appearing multiple times in different .po files but with different translations).

Translations are now read in the following order (from lower to higher priority):

For the 'django' gettext domain:

  • Django translations
  • INSTALLED_APPS apps translations (with the ones listed first having higher priority)
  • settings/project path translations (deprecated, see below)
  • LOCALE_PATHS translations (with the ones listed first having higher priority)

For the 'djangojs' gettext domain:

  • Python modules whose names are passed to the javascript_catalog view
  • LOCALE_PATHS translations (with the ones listed first having higher priority, previously they weren't included)

Also, automatic loading of translations from the 'locale' subdir of the settings/project path is now deprecated.

Thanks to vanschelven, vbmendes and an anonymous user for reporting issues, to vanschelven, Claude Paroz and an anonymous contributor for their initial work on fixes and to Jannis Leidel and Claude for review and discussion.

comment:30 Changed 3 years ago by ramiro

In [15513]:

Added a test for the PendingDeprecationWarning introduced in r15441. Refs #14924, #15286.

comment:3 Changed 3 years ago by ramiro

In [15514]:

Don't merge in translations twice from deprecated project level tree when it is also listed in LOCALE_PATHS. Thanks Claude Paroz. Refs #14924.
Also, removed some old unused variables as reported by pyflakes.

comment:4 Changed 3 years ago by jezdez

In [15529]:

Normalize the locale paths when considering merging a project language catalogue. Refs #14924.

comment:5 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.