[Django] #26328: jsi18n - get_javascript_catalog obscure and hardcoded english
Django
--------------------------------------+--------------------
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 1.9
Severity: Normal | Keywords: jsi18n
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
This is a ticket to get some support in removing the actual hardcoded
english fallback (even if default language is not english) in
get_javascript_catalog.
Discussion here: https://groups.google.com/forum/#!topic/django-
developers/gDU8HlZg4Ug
But basically, the current jsi18n will always load english as a fallback
language, which will be then overriten by what ever your default language
is, but may cause some issues if you don't want to add message strings to
your default language (because ids == values) and you end up with english
translations.
The proposed change is to remove this english loading completely, and only
load the default language as fallback, which will then match the server
side behaviour for translations where the above is not an issue.
Below is the complete code of get_javascript_catalog modified, if there
are no issues with this change I would like to do a pull request for it.
{{{
def get_javascript_catalog(locale, domain, packages):
default_locale = to_locale(settings.LANGUAGE_CODE)
app_configs = apps.get_app_configs()
allowable_packages = set(app_config.name for app_config in
app_configs)
allowable_packages.update(DEFAULT_PACKAGES)
packages = [p for p in packages if p in allowable_packages]
t = {}
paths = []
# paths of requested packages
for package in packages:
p = importlib.import_module(package)
path = os.path.join(os.path.dirname(upath(p.__file__)), 'locale')
paths.append(path)
# add the filesystem paths listed in the LOCALE_PATHS setting
paths.extend(reversed(settings.LOCALE_PATHS))
# First load the settings.LANGUAGE_CODE translations as fallback
for path in paths:
try:
catalog = gettext_module.translation(domain, path,
[default_locale])
except IOError:
catalog = None
if catalog is not None:
t.update(catalog._catalog)
# finally load the currently selected language, if it isn't identical
to the default.
if locale != default_locale:
for path in paths:
try:
catalog = gettext_module.translation(domain, path,
[locale])
except IOError:
catalog = None
if catalog is not None:
t.update(catalog._catalog)
plural = None
if '' in t:
for l in t[''].split('\n'):
if l.startswith('Plural-Forms:'):
plural = l.split(':', 1)[1].strip()
#This is to prevent the unnecessary if condition on the below for
loop.
del t['']
if plural is not None:
# this should actually be a compiled function of a typical plural-
form:
# Plural-Forms: nplurals=3; plural=n%10==1 && n%100!=11 ? 0 :
# n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1
: 2;
plural = [el.strip() for el in plural.split(';') if
el.strip().startswith('plural=')][0].split('=', 1)[1]
pdict = {}
maxcnts = {}
catalog = {}
for k, v in t.items():
if isinstance(k, six.string_types):
catalog[k] = v
elif isinstance(k, tuple):
msgid = k[0]
cnt = k[1]
maxcnts[msgid] = max(cnt, maxcnts.get(msgid, 0))
pdict.setdefault(msgid, {})[cnt] = v
else:
raise TypeError(k)
for k, v in pdict.items():
catalog[k] = [v.get(i, '') for i in range(maxcnts[msgid] + 1)]
return catalog, plural
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26328>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
Keywords: jsi18n | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: => 0
* needs_tests: => 0
* needs_better_patch: => 0
Old description:
New description:
This is a ticket to get some support in removing the actual hardcoded
english fallback (even if default language is not english) in
get_javascript_catalog.
Discussion here: https://groups.google.com/forum/#!topic/django-
developers/gDU8HlZg4Ug
But basically, the current jsi18n will always load english as a fallback
language, which will be then overriten by what ever your default language
is, but may cause some issues if you don't want to add message strings to
your default language (because ids == values) and you end up with english
translations.
The proposed change is to remove this english loading completely, and only
load the default language as fallback, which will then match the server
side behaviour for translations where the above is not an issue.
--
Comment:
Please submit a tested pull requested. I removed the code from the patch's
description since I don't think anyone will review it there (not to
mention, we cannot easily tell what changed).
I'm not sure what you're trying to say by describing the function as
"obscure" in the ticket summary. Could you try to rephrase it to describe
the desired changes?
Finally, if English fallbacks aren't loaded anymore, couldn't this be
backwards-incompatible for some user? (keep in mind, I don't have any
experience with this code so maybe there's no problem)
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:1>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
A related ticket is #26319 -- can you say if your changes would also solve
that issue? Maybe you can dialog with that ticket author a bit.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:2>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
Sorry, first ticket, I guess I lost a lot of information here.
With 'obscure' I tried to say that the code is hard to understand, at
least for me it is not very obvious why it loads english as default or
discards translations if it doesn't find english translations.
About backwards compatibility, there will be a difference in what happens
if a translation for the requested language is not found, and the default
language is not 'en'. Right now if a translation string is not found in
the requested language it will fallback to the default language if it has
a value defined and if it is not found it will fallback to 'en'. The new
behaviour would not fallback to 'en' but rather to the message id, just
like the server side version does (although the server side version also
has some oddly hardcoded english to not load any fallback if the requested
language is english)
I can't tell for sure if it will fix #26319 since it is not obvious if he
is having an issue due to the english special treatment or due to
configuration issues. However with the removed english treatment I can't
see any reason of why specific english locales wouldn't be loaded.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:3>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
Work done here:
https://github.com/cristianocca/django/commit/c6ec94864210406c95ffe5c9a9552c83137e7e1e
Is this the right way to proceed? Should I make this a pull request after?
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:4>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
If it passes the unit tests sure. Can you add a test for the new behavior
that's enabled by the change?
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:5>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
I can not seem to run the whole i18n tests completely, for some reason I'm
getting many errors related to the makemessages command and msgmerge (see
example below) with and without my changes. Oddly when using the real
1.9.4 installation I don't get these errors (it still fails with 2 fails
and 2 errors).
I'm running windows 8, python2 and a virtual enviorment with all
requirements.txt installed.
More in depth results of i18n tests:
i18n.tests --> ok
i18n.test_compilation --> 1 error
i18n.test_extraction --> 26 errors
i18n.test_percents --> ok
I can not find any javascript related test in those. I did however find a
very good ticket that explains why the special english treatment for
translations ( https://code.djangoproject.com/ticket/24413 ).
So the change would actually bring back that issue in order to fix the
other one.
The real issue is not english, but is actually what happens when you have
translations with no values where you expect to get the message id back
but the default language is different from the one with no values, which
will happen very often when mixing apps (like the admin app)
Example...
- default language = 'es'
- My project has 'es' and 'en' translation files, but I'm lazy and I don't
add any translation values for the 'es' ones because it matches the ids
and it is the same as my default language. So if I request an 'es'
translation, since requested == default, no fallback is added and I always
get the expected output.
- Add a new app, which is implemented in english so the developers decided
to have all english translation files with no values, and when they tested
it all worked fine because their default language was english as well. Now
when I use this new app in my project, since my default is 'es', all
requests in english will have 'es' as fallback so if any of those 'es'
translations have a value, I will end up with spanish rather than english,
which is not expected. This is where the team "fixed" the issue by
manually checking for english, and so everything worked fine for all
django apps, but the issue won't be fixed for other languages rather than
english and even worse bring up new issues due to english having a special
treatment.
What can we do? I think the only real way to prevent this is to make sure
all translations are always defined rather than relying on message ids and
remove the special english treatment (this ticket is to remove it from
javascript translations, but server side there's check for english).
Any other ideas? Should be there an app specific setting that will tell
the translation machinery which translations files are missing its values
by purpose because message ids are enough? That way we could actually load
values for those files either at compile time or load time. This would
allow us to remove any english special treatment while still having all
languages load fine for those that are lazy and do not define translation
values, but I have no idea how could this be done.
sample error from tests
{{{
ERROR: test_no_wrap_enabled (i18n.test_extraction.NoWrapExtractorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"C:\Users\Cristiano\Desktop\django_git\django\tests\i18n\test_extraction.py",
line 605, in test_no_wrap_enabled
management.call_command('makemessages', locale=[LOCALE], verbosity=0,
no_wrap=True)
File "C:\Users\Cristiano\Desktop\django_git\venv\lib\site-
packages\django\core\management\__init__.py", line 117, in call_command
return command.execute(*args, **defaults)
File "C:\Users\Cristiano\Desktop\django_git\venv\lib\site-
packages\django\core\management\base.py", line 348, in execute
output = self.handle(*args, **options)
File "C:\Users\Cristiano\Desktop\django_git\venv\lib\site-
packages\django\core\management\commands\makemessages.py", line 307, in
handle
self.write_po_file(potfile, locale)
File "C:\Users\Cristiano\Desktop\django_git\venv\lib\site-
packages\django\core\management\commands\makemessages.py", line 541, in
write_po_file
"errors happened while running msgmerge\n%s" % errors)
CommandError: errors happened while running msgmerge
C:\Users\Cristiano\Desktop\django_git\django\tests\i18n\commands\locale\de\LC_ME
SSAGES\django.po:2:47: syntax error
msgmerge: se encontr\xf3 el error fatal 1
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:6>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* cc: claudep (added)
Comment:
The Windows failures starting in
https://code.djangoproject.com/ticket/25677#comment:10. We need help there
as we don't have any Windows expertise besides Ramiro who hasn't had a
chance to look at the issue. If you can't solve the issue, just ignore the
failures. The test for `javascript_catalog` seem to be in `view_tests`.
I'm adding Claude to CC -- maybe he can advise how to proceed with this
ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:7>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
As expected, tests are failing, there are 3 particular tests that tries
english translations with a default different than english.
If there's an easy way to mark a translation file as "values == ids" so
then it is loaded that way for those files, we could get rid of all the
english special treatments while keeping the current behaviour. However
that seems like it would not be backwards compatible since for example all
the english empty translation files of django should be marked like so.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:8>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
I would like to add, the test calls my attention that it almost seems like
translations are supposed to always have the messages ids in english, look
at this:
{{{
def test_jsi18n_with_missing_en_files(self):
"""
The javascript_catalog shouldn't load the fallback language in the
case that the current selected language is actually the one
translated
from, and hence missing translation files completely.
This happens easily when you're translating from English to other
languages and you've set settings.LANGUAGE_CODE to some other
language
than English.
"""
with self.settings(LANGUAGE_CODE='es'), override('en-us'):
response = self.client.get('/jsi18n/')
self.assertNotContains(response, 'esto tiene que ser
traducido')
}}}
If I read it correctly, it makes the default language 'es' and tries to
get an english translation, and the expected result is to get back an
english translation EVEN if the english locale is not even defined (look
at the languages inside the locale folder of the tests). So basically the
expected behaviour is to get a translation for a language that is not even
defined and make the test pass by giving english special treatment in the
framework.
"The javascript_catalog shouldn't load the fallback language in the case
that the current selected language is actually the one translated from,
and hence missing translation files completely." --> The logic behind this
makes sense, but the actual implementation is completely different (only
favoring english) and not keeping in mind that you may have multiple apps
with different "base" language so you can't relly on a single setting
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:9>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by claudep):
I admit that the javascript i18n definitely needs work. I've tried working
on some refactoring in [https://github.com/django/django/pull/6248 this
PR]. I'm not sure it addresses all concerns of this ticket, but it should
be a start. Reviewers welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:10>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by cristianocca):
It looks better with the changes, but it seems that english is still
having a special treatment. Is there something you can do in this PR to
also remove that?
Do you think you will have these changes done before 1.10? What are you
missing?
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:11>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Comment (by claudep):
I just pushed a revision of the PR. And yes, if the reviews are positive,
it should be committed soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:12>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Component: Internationalization | Version: 1.9
Keywords: jsi18n | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:13>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
Keywords: jsi18n | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:14>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: new
Internationalization |
Severity: Normal | Resolution:
Keywords: jsi18n | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"11c60b529816c5d29c3b4145055bf59b2e389de5" 11c60b5]:
{{{
#!CommitTicketReference repository=""
revision="11c60b529816c5d29c3b4145055bf59b2e389de5"
Reused the DjangoTranslation class for the javascript_catalog view
Thanks Tim Graham and Cristiano Coelho for the reviews.
Refs #26328, #26319.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:15>
Django
Reporter: cristianocca | Owner: nobody
Type: Bug | Status: closed
Component: | Version: 1.9
Internationalization |
Severity: Normal | Resolution: fixed
Keywords: jsi18n | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by claudep):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/26328#comment:16>