Opened 14 years ago
Closed 13 years ago
#15918 closed Cleanup/optimization (fixed)
settings.THOUSAND_SEPARATOR is used only when the current locale does not provide a value
Reported by: | lev | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | 1.3 |
Severity: | Normal | Keywords: | THOUSAND_SEPARATOR |
Cc: | lev.maximov@…, martin.paquette@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
settings.THOUSAND_SEPARATOR is always ignored when rendering templates.
The value from conf/locale is used instead.
Attachments (4)
Change History (23)
by , 14 years ago
Attachment: | thousand_sep.patch added |
---|
comment:1 by , 14 years ago
Needs tests: | set |
---|---|
Resolution: | → invalid |
Status: | new → closed |
comment:2 by , 14 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Docstring doesn't state that settings.THOUSAND_SEPARATOR take precedence over locale. Neither does the documentation. And if you look into the code, the intention is that settings.<WHATEVER> should take precedence over the locale:
try: return _format_cache[cache_key] or getattr(settings, format_type) except KeyError: <take from conf/locale>
But the problem is that it never takes format_type from settings.
Because if the value is not cached yet, it throws KeyError, and if it is cached (from conf/locale), it is taken from cache.
It should be something like
try: return _format_cache.get('cache_key', '') or getattr(settings, format_type) except KeyError: <take from conf/locale>
A cleaner variant is suggested in a patch (which deals better with Nones and empty strings).
comment:3 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | reopened → closed |
Yes, you're right, the documentation is not clear, but the docstring is unambiguous. I'm supposed to use custom format files for that.
comment:4 by , 14 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Type: | Bug → Uncategorized |
Not a bug formally. Still it is somewhat non-intuitive:
1) As suggested by aaugustin, I set USE_I18N = False, USE_L10N = True, THOUSAND_SEPARATOR = ' '
And it still uses THOUSAND_SEPARATOR from locale/en.
That's because it uses THOUSAND_SEPARATOR from settings only if it is missing from conf/locale (which in fact is the case for 8 locales out of 64 currently) - as well as from user/locale.
Not a particularly useful setting. And this behaviour is not documented.
2)
or getattr(settings, format_type)
is never executed and is misleading.
comment:5 by , 14 years ago
Component: | Internationalization → Documentation |
---|---|
Easy pickings: | unset |
Has patch: | unset |
Needs documentation: | set |
Needs tests: | unset |
Summary: | settings.THOUSAND_SEPARATOR is ignored on output → settings.THOUSAND_SEPARATOR is used only when the current locale does not provide a value |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
lev, I'm glad to see that we finally agree: the problem is that the THOUSAND_SEPARATOR
(and other l10n related settings) are a) not generally useful b) not sufficiently documented.
I tend to think they were added only to avoid hardcoding default values and to make the code in formats.py more generic.
Anyway, the behavior is not obviously buggy, so we'll run into backwards compatibility issues if we change it. I'll classify this as a documentation issue: the docs should say that these settings are used only if the current locale does not provide a value. I'd appreciate if someone who knows why they were introduced commented on this.
For your information, I had written the text below while you were adding your own comments.
Let's step back for a minute. It is very difficult to assess bugs based on interpretations of "code intention". This discussion may very well take us nowhere. If you think there is a bug, you should exhibit a test case with a behavior that is either obviously wrong, or incompatible with the docs. That would make a good basis for a discussion.
Now, regarding your comment:
But the problem is that it never takes format_type from settings.
The first call to get_format()
does in the following scenario:
_format_cache[cache_key]
raisesKeyError
- no module from get_format_modules() has the required attribute, so
AttributeError
is raised every time in the loop - line 81 gets executed:
_format_cache[cache_key] = None
- line 82 gets executed:
return getattr(settings, format_type)
: the value from the settings is used
During further calls to get_format()
, at line 72, _format_cache[cache_key]
will be None
, so the function will return getattr(settings, format_type)
: the value from the settings is used again. [edit: this adresses 2) in your last comment].
comment:6 by , 14 years ago
Thanks for your thorough reply.
Concerning (2): yes, that's right, that code is not useless.
And still that's an odd way to organize a cache.
I can't imagine what prevents them from caching that value from settings as well the first time they discover that the key is missing from locale/*/formats. Maybe they plan attaching new formats modules on the fly one day? No, that's just ok, it's entirely up to them )
comment:7 by , 14 years ago
Lev, thank you for opening this ticket!
Untill your patch got applied, one could monkey patch Django to achieve desired behaviour:
try: from django.utils.formats import _format_cache cache_key = ('THOUSAND_SEPARATOR', 'ru') # change 'ru' to your locale _format_cache[cache_key] = "'" except: pass # use default settings
comment:8 by , 14 years ago
Has patch: | set |
---|---|
Needs tests: | set |
comment:9 by , 14 years ago
Component: | Documentation → Internationalization |
---|
comment:10 by , 14 years ago
Andy, thanks for your reply. As aaugustin
pointed out, my patch is not necessary in fact. Current django code is ok, it is only the documentation that is unclear.
My current solution looks like this (in accordance with docs):
settings.py: FORMAT_MODULE_PATH = 'mysite.formats' USE_L10N = True USE_THOUSAND_SEPARATOR = True mysite/formats/ru/formats.py: DECIMAL_SEPARATOR = '.' mysite/formats/en/formats.py: THOUSAND_SEPARATOR = ' ' (plus empty __init__.py in corresponding dirs of course)
This way I get a somewhat consistent behaviour of this feature for the locales I work with, which is exactly what I wanted.
comment:11 by , 14 years ago
What is actually missing in the documentation is that out of 4 variables responsible for localized number rendering (DECIMAL_SEPARATOR
, THOUSAND_SEPARATOR
, NUMBER_GROUPING
, USE_THOUSAND_SEPARATOR
) only the last one is intended for being used in settings.py
directly.
The other three are supposed to be set in the corresponding formats.py
files like in the example above.
They can be set in settings.py
but they will only be used in an unlikely case of the variable not set up in django/conf/locale/*/formats.py
files.
For example the very first locale, ar
, has DECIMAL_SEPARATOR
set, but it will not be used unless NUMBER_GROUPING
(which is suspiciously commented out there) is set somewhere else (ie in settings.py
or in mysite/formats/ar/formats.py
).
comment:12 by , 14 years ago
Component: | Internationalization → Documentation |
---|---|
Needs documentation: | unset |
Needs tests: | unset |
After checking with jezdez on IRC, I'm marking this as a documentation issue again.
I'm attaching a patch based on lev's last comment, which is a good summary of the situation.
by , 14 years ago
Attachment: | 15918.patch added |
---|
comment:13 by , 14 years ago
Patch needs improvement: | set |
---|
15918.patch fails to apply cleanly on to trunk
by , 14 years ago
Attachment: | 15918.2.patch added |
---|
comment:14 by , 14 years ago
Patch needs improvement: | unset |
---|
Patch was relative to trunk/docs
instead of trunk
. Thanks patchhammer / dmclain :)
comment:15 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:17 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
UI/UX: | unset |
We should read USE_L10N instead of USE_I18N.
comment:18 by , 13 years ago
Cc: | added |
---|
comment:19 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Thanks, this was rectified in r16850.
In the docstring of
django.utils.formats.get_format
, I read that the value from the settings is intended only for the cases where no locale is defined, which probably meansUSE_L10N = True
andUSE_I18N = False
.I assume you have I18N enabled, and this is why the value from
settings.THOUSAND_SEPARATOR
is not used. Please provide more details and reopen the ticket if I have misunderstood your problem.The current code uses
_format_cache[cache_key] or getattr(settings, format_type)
, and it looks correct.settings.<WHATEVER>
is a default value that will be used if no format could be found by lines 74-80, which leads to_format_cache[cache_key] = None
the next time the function is called.