Opened 6 years ago
Closed 4 years ago
#29712 closed Cleanup/optimization (fixed)
Add warning in makemessages command if the localecode with `l` flag is not correct
Reported by: | Sanyam Khurana | Owned by: | Manav Agarwal |
---|---|---|---|
Component: | Internationalization | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Herbert Fortes | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
Hey Calude,
What about normalizing the directory name to something that would just work.
For example,
No matter, if the developer is doing all these:
python manage.py makemessages -l zh_cn
python manage.py makemessages -l zh_CN
python manage.py makemessages -l ZH_CN
python manage.py makemessages -l ZH-CN
etc.
we, just normalize the directory name to zh_CN
and it would work.
I'm still about to read the code of makemessages
command and probably if there are any more checks than just this, then we'll have to figure out another way all together.
Change History (44)
comment:1 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 6 years ago
Description: | modified (diff) |
---|
comment:5 by , 6 years ago
Owner: | set to |
---|
comment:6 by , 6 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Type: | Bug → Cleanup/optimization |
Version: | 2.1 → master |
comment:7 by , 6 years ago
Cc: | added |
---|---|
Owner: | changed from | to
comment:10 by , 6 years ago
I think I would still output a warning when a language code is normalized, just to inform the user that its input has been corrected.
comment:12 by , 6 years ago
Trying to coerce any input to something like a language tag only to hack it back to a locale using to_locale()
feels like a kludge. It would be better to improve documentation.
follow-up: 15 comment:13 by , 6 years ago
Improving documentation is welcome, but silently accepting a wrong language code also look a bit suspicious. I think I would be happy with a warning without coercing anything.
comment:14 by , 6 years ago
We definitely need to improve the documentations. Coercing the language code is something we have to take call on.
comment:15 by , 6 years ago
Replying to Claude Paroz:
Improving documentation is welcome, but silently accepting a wrong language code also look a bit suspicious. I think I would be happy with a warning without coercing anything.
I agree. I think a warning would make sense, without coercion.
It is still possible to provide a locale to makemessages
where there are no actual message catalogs in any of the paths in settings.LOCALE_PATHS
.
We should probably scrap all the normalization stuff and just output a warning message if a locale specified by the user is not in all_locales
.
At the moment we output a "processing locale xx_XX"
message if verbosity > 0
which should be fixed to only happen for valid, existing locales.
As an aside, this is checking --locale
for makemessages
, but what about compilemessages
? (And are their any others?)
comment:16 by , 6 years ago
Now I am thinking more to avoid the coercing and to put just a warning message.
comment:17 by , 6 years ago
Patch needs improvement: | set |
---|
comment:18 by , 6 years ago
Tim
Current implementation involved just adding the warning message. We are not normalizing locale now.
comment:19 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:21 by , 6 years ago
Patch needs improvement: | set |
---|
As commented on the pull request, you cannot use the all_locales
variable as a reference as it does not contain new (valid) language codes. The modified test in your patch shows a 'Invalid locale en'
message which is obviously wrong, isn't it?
You may try using the django.utils.translation.trans_real.language_code_re
to check the locale code syntax.
comment:23 by , 6 years ago
Hi Sanyam,
Yes, I am working on this ticket but your help would be very appreciated.
comment:24 by , 6 years ago
Cc: | removed |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:27 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:28 by , 6 years ago
Cc: | added |
---|
comment:29 by , 6 years ago
Hi,
Here my thoughts:
language_code_re gets 'pt_BR'. And it does not get 'ZH-CN'. I did this:
r'[a-z]{2}(_[A-Z]{2})?'
It seems to work. But one test prints:
System check identified no issues (0 silenced). Invalid locale d Invalid locale e ................................................. ---------------------------------------------------------------------- Ran 49 tests in 1.425s OK
I did a check and there is only one line - 740 - that's use locale=LOCALE
. All
others use locale=[LOCALE]
.
What do you think Mark?
comment:30 by , 6 years ago
I forgot to say that the regrex I sent fails to:
ja_JP_JP no_NO_NY th_TH_TH
comment:31 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I'm going to de-assign this because there's been progress, so that someone looking can pick it up.
comment:32 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:34 by , 4 years ago
Hi Manav, please do, no need to ask (but can I advise that you take just one at a time to begin :)
comment:35 by , 4 years ago
Owner: | changed from | to
---|
comment:36 by , 4 years ago
Can we use LANG_INFO from django.conf.locale
in order to solve the conflict and raise the warning or as an alternative of language_code_re
? If yes, then please confirm so that I may submit a patch, and also please suggest some way to use the same.
OR
We could even use the regex from language_code_re
which is re.compile(r'^[a-z]{1,8}(?:-[a-z0-9]{1,8})*(?:@[a-z0-9]{1,20})?$', re.IGNORECASE)
to solve the conflict and raise the warning.
Please suggest the best option to work on, for the patch.
comment:37 by , 4 years ago
The problem with using LANG_INFO
is that you exclude some extra languages added specifically for a project.
You can try to use language_code_re
, but you'll have to take into account the language code vs locale code difference.
follow-up: 40 comment:39 by , 4 years ago
Needs tests: | set |
---|
comment:40 by , 4 years ago
If the patch in PR seems fine then please update me so that I may start working on tests.
comment:41 by , 4 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:42 by , 4 years ago
Patch needs improvement: | set |
---|
comment:43 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:44 by , 4 years ago
Patch needs improvement: | set |
---|
comment:45 by , 4 years ago
As per the comment by felixxm on PR, I have created a check for the valid locales to not have hyphens.
comment:46 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
We have to be careful in deciding what is a "good" and a "bad" code. However I agree that we can avoid some mistakes, notably the confusion between IETF language tags [1] and ISO/IEC 15897 (Posix) [2] codes generally expected by Django.
[1] https://en.wikipedia.org/wiki/IETF_language_tag
[2] https://en.wikipedia.org/wiki/Locale_(computer_software)