Opened 2 years ago

Last modified 10 days ago

#29712 new Cleanup/optimization

Add warning in makemessages command if the localecode with `l` flag is not correct

Reported by: Sanyam Khurana Owned by:
Component: Internationalization Version: master
Severity: Normal Keywords:
Cc: Herbert Fortes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (28)

comment:1 Changed 2 years ago by David

Owner: changed from nobody to David
Status: newassigned

comment:2 Changed 2 years ago by Claude Paroz

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 Changed 2 years ago by Sanyam Khurana

Description: modified (diff)

comment:5 Changed 2 years ago by Matt Segal

Owner: set to Matt Segal

comment:6 Changed 2 years ago by Claude Paroz

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

comment:7 Changed 2 years ago by Vishvajit Pathak

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

comment:8 Changed 2 years ago by Vishvajit Pathak

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

comment:9 Changed 2 years ago by Vishvajit Pathak

Needs tests: unset

comment:10 Changed 2 years ago by Claude Paroz

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 Changed 2 years ago by Vishvajit Pathak

Warning added when a language code is normalized.

comment:12 Changed 2 years ago by Nick Pope

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 Changed 2 years ago by 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.

comment:14 Changed 2 years ago by Vishvajit Pathak

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

comment:15 in reply to:  13 Changed 2 years ago by Nick Pope

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 Changed 2 years ago by Vishvajit Pathak

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

comment:17 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:18 Changed 2 years ago by Vishvajit Pathak

Tim

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

comment:19 Changed 23 months ago by Tim Graham

Patch needs improvement: unset

comment:20 Changed 23 months ago by Vishvajit Pathak

Tim,

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

comment:21 Changed 23 months ago by Claude Paroz

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 Changed 22 months ago by Sanyam Khurana

Hi Vishvajit!

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

comment:23 Changed 22 months ago by Vishvajit Pathak

Hi Sanyam,

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

comment:24 Changed 21 months ago by Vishvajit Pathak

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

comment:27 Changed 21 months ago by Mark Dawson

Owner: set to Mark Dawson
Status: newassigned

comment:28 Changed 21 months ago by Herbert Fortes

Cc: Herbert Fortes added

comment:29 Changed 21 months ago by Herbert Fortes

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 Changed 21 months ago by Herbert Fortes

I forgot to say that the regrex I sent fails to:

ja_JP_JP
no_NO_NY
th_TH_TH

comment:31 Changed 17 months ago by Carlton Gibson

Owner: Mark Dawson deleted
Status: assignednew

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

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