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 Sanyam Khurana)

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 David, 6 years ago

Owner: changed from nobody to David
Status: newassigned

comment:2 by Claude Paroz, 6 years ago

Triage Stage: UnreviewedAccepted

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)

comment:4 by Sanyam Khurana, 6 years ago

Description: modified (diff)

comment:5 by Matt Segal, 6 years ago

Owner: set to Matt Segal

comment:6 by Claude Paroz, 6 years ago

Has patch: set
Needs tests: set
Type: BugCleanup/optimization
Version: 2.1master
Last edited 6 years ago by Tim Graham (previous) (diff)

comment:7 by Vishvajit Pathak, 6 years ago

Cc: Vishvajit Pathak added
Owner: changed from Matt Segal to Vishvajit Pathak

comment:8 by Vishvajit Pathak, 6 years ago

Last edited 6 years ago by Vishvajit Pathak (previous) (diff)

comment:9 by Vishvajit Pathak, 6 years ago

Needs tests: unset

comment:10 by Claude Paroz, 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:11 by Vishvajit Pathak, 6 years ago

Warning added when a language code is normalized.

comment:12 by Nick Pope, 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.

comment:13 by Claude Paroz, 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 Vishvajit Pathak, 6 years ago

We definitely need to improve the documentations. Coercing the language code is something we have to take call on.

in reply to:  13 comment:15 by Nick Pope, 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 Vishvajit Pathak, 6 years ago

Now I am thinking more to avoid the coercing and to put just a warning message.

comment:17 by Tim Graham, 6 years ago

Patch needs improvement: set

comment:18 by Vishvajit Pathak, 6 years ago

Tim

Current implementation involved just adding the warning message. We are not normalizing locale now.

comment:19 by Tim Graham, 6 years ago

Patch needs improvement: unset

comment:20 by Vishvajit Pathak, 6 years ago

Tim,

I would like to understand better what needs to be done next ?

comment:21 by Claude Paroz, 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:22 by Sanyam Khurana, 6 years ago

Hi Vishvajit!

Are you still working on the bug? Do you need any help?

comment:23 by Vishvajit Pathak, 6 years ago

Hi Sanyam,

Yes, I am working on this ticket but your help would be very appreciated.

comment:24 by Vishvajit Pathak, 6 years ago

Cc: Vishvajit Pathak removed
Owner: Vishvajit Pathak removed
Status: assignednew

comment:27 by Mark Dawson, 6 years ago

Owner: set to Mark Dawson
Status: newassigned

comment:28 by Herbert Fortes, 6 years ago

Cc: Herbert Fortes added

comment:29 by Herbert Fortes, 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 Herbert Fortes, 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 Carlton Gibson, 6 years ago

Owner: Mark Dawson removed
Status: assignednew

I'm going to de-assign this because there's been progress, so that someone looking can pick it up.

comment:32 by Aniket118, 4 years ago

Owner: set to Aniket118
Status: newassigned

comment:33 by Manav Agarwal, 4 years ago

Can I claim this? I think I can move this forward

comment:34 by Carlton Gibson, 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 Manav Agarwal, 4 years ago

Owner: changed from Aniket118 to Manav Agarwal

comment:36 by Manav Agarwal, 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.

Last edited 4 years ago by Manav Agarwal (previous) (diff)

comment:37 by Claude Paroz, 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.

comment:38 by Manav Agarwal, 4 years ago

comment:39 by Manav Agarwal, 4 years ago

Needs tests: set

in reply to:  39 comment:40 by Manav Agarwal, 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 Manav Agarwal, 4 years ago

Needs tests: unset
Patch needs improvement: unset

comment:42 by Jacob Walls, 4 years ago

Patch needs improvement: set

comment:43 by Jacob Walls, 4 years ago

Patch needs improvement: unset

comment:44 by Mariusz Felisiak, 4 years ago

Patch needs improvement: set

comment:45 by Manav Agarwal, 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 Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:47 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In f63f3cdf:

Fixed #29712 -- Made makemessages warn if locales have hyphens and skip them.

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