Opened 13 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)

thousand_sep.patch (641 bytes ) - added by lev.maximov@… 13 years ago.
15918.patch (3.1 KB ) - added by Aymeric Augustin 13 years ago.
15918.2.patch (3.1 KB ) - added by Aymeric Augustin 13 years ago.
15918.3.diff (561 bytes ) - added by Martin Paquette 13 years ago.
Replace i18n by l10n

Download all attachments as: .zip

Change History (23)

by lev.maximov@…, 13 years ago

Attachment: thousand_sep.patch added

comment:1 by Aymeric Augustin, 13 years ago

Needs tests: set
Resolution: invalid
Status: newclosed

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 means USE_L10N = True and USE_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.

comment:2 by lev, 13 years ago

Resolution: invalid
Status: closedreopened

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 lev, 13 years ago

Resolution: invalid
Status: reopenedclosed

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 lev, 13 years ago

Resolution: invalid
Status: closedreopened
Type: BugUncategorized

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 Aymeric Augustin, 13 years ago

Component: InternationalizationDocumentation
Easy pickings: unset
Has patch: unset
Needs documentation: set
Needs tests: unset
Summary: settings.THOUSAND_SEPARATOR is ignored on outputsettings.THOUSAND_SEPARATOR is used only when the current locale does not provide a value
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/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] raises KeyError
  • 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 lev, 13 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 andy, 13 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 Jannis Leidel, 13 years ago

Has patch: set
Needs tests: set

comment:9 by Jannis Leidel, 13 years ago

Component: DocumentationInternationalization

comment:10 by lev, 13 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 lev, 13 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 Aymeric Augustin, 13 years ago

Component: InternationalizationDocumentation
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 Aymeric Augustin, 13 years ago

Attachment: 15918.patch added

comment:13 by patchhammer, 13 years ago

Patch needs improvement: set

15918.patch fails to apply cleanly on to trunk

by Aymeric Augustin, 13 years ago

Attachment: 15918.2.patch added

comment:14 by Aymeric Augustin, 13 years ago

Patch needs improvement: unset

Patch was relative to trunk/docs instead of trunk. Thanks patchhammer / dmclain :)

comment:15 by dmclain, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: reopenedclosed

In [16727]:

Fixed #15918 -- Refined documentation of the various localization settings, especially with regard to the thousand separator. Thanks, Aymeric Augustin.

by Martin Paquette, 13 years ago

Attachment: 15918.3.diff added

Replace i18n by l10n

comment:17 by Martin Paquette, 13 years ago

Resolution: fixed
Status: closedreopened
UI/UX: unset

We should read USE_L10N instead of USE_I18N.

I'm attaching patch 15918.3.diff

Last edited 13 years ago by Martin Paquette (previous) (diff)

comment:18 by Martin Paquette, 13 years ago

Cc: martin.paquette@… added

comment:19 by Julien Phalip, 13 years ago

Resolution: fixed
Status: reopenedclosed

Thanks, this was rectified in r16850.

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