Code

Opened 3 years ago

Closed 3 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@… 3 years ago.
15918.patch (3.1 KB) - added by aaugustin 3 years ago.
15918.2.patch (3.1 KB) - added by aaugustin 3 years ago.
15918.3.diff (561 bytes) - added by martinpaquette 3 years ago.
Replace i18n by l10n

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by lev.maximov@…

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 3 years ago by lev

  • Resolution invalid deleted
  • Status changed from closed to 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 Changed 3 years ago by lev

  • Resolution set to invalid
  • Status changed from reopened to 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 Changed 3 years ago by lev

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Type changed from Bug to 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 Changed 3 years ago by aaugustin

  • Component changed from Internationalization to Documentation
  • Easy pickings unset
  • Has patch unset
  • Needs documentation set
  • Needs tests unset
  • Summary changed from settings.THOUSAND_SEPARATOR is ignored on output to settings.THOUSAND_SEPARATOR is used only when the current locale does not provide a value
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to 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] 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 Changed 3 years ago by lev

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 Changed 3 years ago by andy

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 Changed 3 years ago by jezdez

  • Has patch set
  • Needs tests set

comment:9 Changed 3 years ago by jezdez

  • Component changed from Documentation to Internationalization

comment:10 Changed 3 years ago by lev

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 Changed 3 years ago by lev

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 Changed 3 years ago by aaugustin

  • Component changed from Internationalization to 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.

Changed 3 years ago by aaugustin

comment:13 Changed 3 years ago by patchhammer

  • Patch needs improvement set

15918.patch fails to apply cleanly on to trunk

Changed 3 years ago by aaugustin

comment:14 Changed 3 years ago by aaugustin

  • Patch needs improvement unset

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

comment:15 Changed 3 years ago by dmclain

  • Triage Stage changed from Accepted to Ready for checkin

comment:16 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

In [16727]:

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

Changed 3 years ago by martinpaquette

Replace i18n by l10n

comment:17 Changed 3 years ago by martinpaquette

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • UI/UX unset

We should read USE_L10N instead of USE_I18N.

I'm attaching patch 15918.3.diff

Last edited 3 years ago by martinpaquette (previous) (diff)

comment:18 Changed 3 years ago by martinpaquette

  • Cc martin.paquette@… added

comment:19 Changed 3 years ago by julien

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks, this was rectified in r16850.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.