Django: Ticket #15918: settings.THOUSAND_SEPARATOR is used only when the current locale does not provide a valuehttps://code.djangoproject.com/ticket/15918
<p>
settings.THOUSAND_SEPARATOR is always ignored when rendering templates.
The value from conf/locale is used instead.
</p>
en-usDjangohttps://www.djangoproject.com/s/img/site/hdr_logo.gifhttps://code.djangoproject.com/ticket/15918
Trac 1.0.2lev.maximov@…Thu, 28 Apr 2011 14:02:30 GMTattachment sethttps://code.djangoproject.com/ticket/15918
https://code.djangoproject.com/ticket/15918
<ul>
<li><strong>attachment</strong>
set to <em>thousand_sep.patch</em>
</li>
</ul>
TicketaaugustinThu, 28 Apr 2011 16:08:15 GMTstatus changed; needs_docs, resolution, needs_tests, needs_better_patch sethttps://code.djangoproject.com/ticket/15918#comment:1
https://code.djangoproject.com/ticket/15918#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>needs_docs</strong>
unset
</li>
<li><strong>resolution</strong>
set to <em>invalid</em>
</li>
<li><strong>needs_tests</strong>
set
</li>
<li><strong>needs_better_patch</strong>
unset
</li>
</ul>
<p>
In the docstring of <tt>django.utils.formats.get_format</tt>, I read that the value from the settings is intended only for the cases where no locale is defined, which probably means <tt>USE_L10N = True</tt> and <tt>USE_I18N = False</tt>.
</p>
<p>
I assume you have I18N enabled, and this is why the value from <tt>settings.THOUSAND_SEPARATOR</tt> is not used. Please provide more details and reopen the ticket if I have misunderstood your problem.
</p>
<p>
The current code uses<tt>_format_cache[cache_key] or getattr(settings, format_type)</tt>, and it looks correct. <tt>settings.&lt;WHATEVER&gt;</tt> is a default value that will be used if no format could be found by lines 74-80, which leads to <tt>_format_cache[cache_key] = None</tt> the next time the function is called.
</p>
TicketlevThu, 28 Apr 2011 17:35:13 GMTstatus changed; resolution deletedhttps://code.djangoproject.com/ticket/15918#comment:2
https://code.djangoproject.com/ticket/15918#comment:2
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>reopened</em>
</li>
<li><strong>resolution</strong>
<em>invalid</em> deleted
</li>
</ul>
<p>
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.&lt;WHATEVER&gt; should take precedence over the locale:
</p>
<pre class="wiki">try:
return _format_cache[cache_key] or getattr(settings, format_type)
except KeyError:
&lt;take from conf/locale&gt;
</pre><p>
But the problem is that it never takes format_type from settings.
</p>
<p>
Because if the value is not cached yet, it throws KeyError, and if it is cached (from conf/locale), it is taken from cache.
</p>
<p>
It should be something like
</p>
<pre class="wiki">try:
return _format_cache.get('cache_key', '') or getattr(settings, format_type)
except KeyError:
&lt;take from conf/locale&gt;
</pre><p>
A cleaner variant is suggested in a patch (which deals better with Nones and empty strings).
</p>
TicketlevThu, 28 Apr 2011 17:49:41 GMTstatus changed; resolution sethttps://code.djangoproject.com/ticket/15918#comment:3
https://code.djangoproject.com/ticket/15918#comment:3
<ul>
<li><strong>status</strong>
changed from <em>reopened</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>invalid</em>
</li>
</ul>
<p>
Yes, you're right, the documentation is not clear, but the docstring is unambiguous. I'm supposed to use custom format files for that.
</p>
TicketlevThu, 28 Apr 2011 19:04:16 GMTstatus, type changed; resolution deletedhttps://code.djangoproject.com/ticket/15918#comment:4
https://code.djangoproject.com/ticket/15918#comment:4
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>reopened</em>
</li>
<li><strong>type</strong>
changed from <em>Bug</em> to <em>Uncategorized</em>
</li>
<li><strong>resolution</strong>
<em>invalid</em> deleted
</li>
</ul>
<p>
Not a bug formally. Still it is somewhat non-intuitive:
</p>
<p>
1) As suggested by aaugustin, I set USE_I18N = False, USE_L10N = True, THOUSAND_SEPARATOR = ' '
</p>
<p>
And it still uses THOUSAND_SEPARATOR from locale/en.
</p>
<p>
That's because it uses THOUSAND_SEPARATOR from settings <strong>only</strong> 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.
</p>
<p>
Not a particularly useful setting. And this behaviour is not documented.
</p>
<p>
2)
</p>
<pre class="wiki">or getattr(settings, format_type)
</pre><p>
is never executed and is misleading.
</p>
TicketaaugustinThu, 28 Apr 2011 20:08:21 GMTcomponent, needs_tests, summary, easy, needs_docs, has_patch, type, stage changedhttps://code.djangoproject.com/ticket/15918#comment:5
https://code.djangoproject.com/ticket/15918#comment:5
<ul>
<li><strong>component</strong>
changed from <em>Internationalization</em> to <em>Documentation</em>
</li>
<li><strong>needs_tests</strong>
unset
</li>
<li><strong>summary</strong>
changed from <em>settings.THOUSAND_SEPARATOR is ignored on output</em> to <em>settings.THOUSAND_SEPARATOR is used only when the current locale does not provide a value</em>
</li>
<li><strong>easy</strong>
unset
</li>
<li><strong>needs_docs</strong>
set
</li>
<li><strong>has_patch</strong>
unset
</li>
<li><strong>type</strong>
changed from <em>Uncategorized</em> to <em>Cleanup/optimization</em>
</li>
<li><strong>stage</strong>
changed from <em>Unreviewed</em> to <em>Accepted</em>
</li>
</ul>
<p>
lev, I'm glad to see that we finally agree: the problem is that the <tt>THOUSAND_SEPARATOR</tt> (and other l10n related settings) are a) not generally useful b) not sufficiently documented.
</p>
<p>
I tend to think they were added only to avoid hardcoding default values and to make the code in formats.py more generic.
</p>
<p>
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.
</p>
<hr />
<p>
For your information, I had written the text below while you were adding your own comments.
</p>
<p>
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.
</p>
<p>
Now, regarding your <a class="ticket" href="https://code.djangoproject.com/ticket/15918#comment:2" title="Comment 2">comment</a>:
</p>
<blockquote class="citation">
<p>
But the problem is that it never takes format_type from settings.
</p>
</blockquote>
<p>
The first call to <tt>get_format()</tt> does in the following scenario:
</p>
<ul><li><tt>_format_cache[cache_key]</tt> raises <tt>KeyError</tt>
</li><li>no module from get_format_modules() has the required attribute, so <tt>AttributeError</tt> is raised every time in the loop
</li><li>line 81 gets executed: <tt>_format_cache[cache_key] = None</tt>
</li><li>line 82 gets executed: <tt>return getattr(settings, format_type)</tt>: the value from the settings is used
</li></ul><p>
During further calls to <tt>get_format()</tt>, at line 72, <tt>_format_cache[cache_key]</tt> will be <tt>None</tt>, so the function will return <tt>getattr(settings, format_type)</tt>: the value from the settings is used again. [edit: this adresses 2) in your last comment].
</p>
TicketlevThu, 28 Apr 2011 21:05:26 GMThttps://code.djangoproject.com/ticket/15918#comment:6
https://code.djangoproject.com/ticket/15918#comment:6
<p>
Thanks for your thorough reply.
</p>
<p>
Concerning (2): yes, that's right, that code is not useless.
</p>
<p>
And still that's an odd way to organize a cache.
</p>
<p>
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 )
</p>
TicketandyMon, 02 May 2011 11:20:25 GMThttps://code.djangoproject.com/ticket/15918#comment:7
https://code.djangoproject.com/ticket/15918#comment:7
<p>
Lev, thank you for opening this ticket!
</p>
<p>
Untill your patch got applied, one could monkey patch Django to achieve desired behaviour:
</p>
<pre class="wiki">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
</pre>
TicketjezdezMon, 02 May 2011 11:46:27 GMThas_patch, needs_tests changedhttps://code.djangoproject.com/ticket/15918#comment:8
https://code.djangoproject.com/ticket/15918#comment:8
<ul>
<li><strong>has_patch</strong>
set
</li>
<li><strong>needs_tests</strong>
set
</li>
</ul>
TicketjezdezMon, 02 May 2011 11:46:36 GMTcomponent changedhttps://code.djangoproject.com/ticket/15918#comment:9
https://code.djangoproject.com/ticket/15918#comment:9
<ul>
<li><strong>component</strong>
changed from <em>Documentation</em> to <em>Internationalization</em>
</li>
</ul>
TicketlevMon, 02 May 2011 12:30:32 GMThttps://code.djangoproject.com/ticket/15918#comment:10
https://code.djangoproject.com/ticket/15918#comment:10
<p>
Andy, thanks for your reply. As <tt>aaugustin</tt> pointed out, my patch is not necessary in fact. Current django code is ok, it is only the documentation that is unclear.
</p>
<p>
My current solution looks like this (in accordance with <a class="ext-link" href="http://docs.djangoproject.com/en/1.3/topics/i18n/localization/#format-localization"><span class="icon">​</span>docs</a>):
</p>
<pre class="wiki">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)
</pre><p>
This way I get a somewhat consistent behaviour of this feature for the locales I work with, which is exactly what I wanted.
</p>
TicketlevMon, 02 May 2011 12:43:33 GMThttps://code.djangoproject.com/ticket/15918#comment:11
https://code.djangoproject.com/ticket/15918#comment:11
<p>
What is actually missing in the documentation is that out of 4 variables responsible for localized number rendering (<tt>DECIMAL_SEPARATOR</tt>, <tt>THOUSAND_SEPARATOR</tt>, <tt>NUMBER_GROUPING</tt>, <tt>USE_THOUSAND_SEPARATOR</tt>) only the last one is intended for being used in <tt>settings.py</tt> directly.
</p>
<p>
The other three are supposed to be set in the corresponding <tt>formats.py</tt> files like in the example above.
</p>
<p>
They <em>can</em> be set in <tt>settings.py</tt> but they will only be used in an unlikely case of the variable not set up in <tt>django/conf/locale/*/formats.py</tt> files.
</p>
<p>
For example the very first locale, <tt>ar</tt>, has <tt>DECIMAL_SEPARATOR</tt> set, but it will not be used unless <tt>NUMBER_GROUPING</tt> (which is suspiciously commented out there) is set somewhere else (ie in <tt>settings.py</tt> or in <tt>mysite/formats/ar/formats.py</tt>).
</p>
TicketaaugustinMon, 02 May 2011 21:04:02 GMTneeds_docs, component, needs_tests changedhttps://code.djangoproject.com/ticket/15918#comment:12
https://code.djangoproject.com/ticket/15918#comment:12
<ul>
<li><strong>needs_docs</strong>
unset
</li>
<li><strong>component</strong>
changed from <em>Internationalization</em> to <em>Documentation</em>
</li>
<li><strong>needs_tests</strong>
unset
</li>
</ul>
<p>
After checking with jezdez on IRC, I'm marking this as a documentation issue again.
</p>
<p>
I'm attaching a patch based on lev's last comment, which is a good summary of the situation.
</p>
TicketaaugustinMon, 02 May 2011 21:04:21 GMTattachment sethttps://code.djangoproject.com/ticket/15918
https://code.djangoproject.com/ticket/15918
<ul>
<li><strong>attachment</strong>
set to <em>15918.patch</em>
</li>
</ul>
TicketpatchhammerThu, 05 May 2011 18:42:40 GMTneeds_better_patch changedhttps://code.djangoproject.com/ticket/15918#comment:13
https://code.djangoproject.com/ticket/15918#comment:13
<ul>
<li><strong>needs_better_patch</strong>
set
</li>
</ul>
<p>
15918.patch fails to apply cleanly on to trunk
</p>
TicketaaugustinThu, 05 May 2011 20:25:32 GMTattachment sethttps://code.djangoproject.com/ticket/15918
https://code.djangoproject.com/ticket/15918
<ul>
<li><strong>attachment</strong>
set to <em>15918.2.patch</em>
</li>
</ul>
TicketaaugustinThu, 05 May 2011 20:26:35 GMTneeds_better_patch changedhttps://code.djangoproject.com/ticket/15918#comment:14
https://code.djangoproject.com/ticket/15918#comment:14
<ul>
<li><strong>needs_better_patch</strong>
unset
</li>
</ul>
<p>
Patch was relative to <tt>trunk/docs</tt> instead of <tt>trunk</tt>. Thanks patchhammer / dmclain :)
</p>
TicketdmclainMon, 09 May 2011 16:39:16 GMTstage changedhttps://code.djangoproject.com/ticket/15918#comment:15
https://code.djangoproject.com/ticket/15918#comment:15
<ul>
<li><strong>stage</strong>
changed from <em>Accepted</em> to <em>Ready for checkin</em>
</li>
</ul>
TicketjezdezThu, 08 Sep 2011 13:25:18 GMTstatus changed; resolution sethttps://code.djangoproject.com/ticket/15918#comment:16
https://code.djangoproject.com/ticket/15918#comment:16
<ul>
<li><strong>status</strong>
changed from <em>reopened</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
In <a class="changeset" href="https://code.djangoproject.com/changeset/16727/">[16727]</a>:
</p>
<div class="message"><p>
Fixed <a class="closed ticket" href="https://code.djangoproject.com/ticket/15918" title="Cleanup/optimization: settings.THOUSAND_SEPARATOR is used only when the current locale does ... (closed: fixed)">#15918</a> -- Refined documentation of the various localization settings, especially with regard to the thousand separator. Thanks, Aymeric Augustin.<br />
</p>
</div>
TicketmartinpaquetteThu, 08 Sep 2011 21:06:03 GMTattachment sethttps://code.djangoproject.com/ticket/15918
https://code.djangoproject.com/ticket/15918
<ul>
<li><strong>attachment</strong>
set to <em>15918.3.diff</em>
</li>
</ul>
<p>
Replace i18n by l10n
</p>
TicketmartinpaquetteThu, 08 Sep 2011 21:09:02 GMTstatus changed; ui_ux set; resolution deletedhttps://code.djangoproject.com/ticket/15918#comment:17
https://code.djangoproject.com/ticket/15918#comment:17
<ul>
<li><strong>status</strong>
changed from <em>closed</em> to <em>reopened</em>
</li>
<li><strong>ui_ux</strong>
unset
</li>
<li><strong>resolution</strong>
<em>fixed</em> deleted
</li>
</ul>
<p>
We should read USE_L10N instead of USE_I18N.
</p>
<p>
I'm attaching patch 15918.3.diff
</p>
TicketmartinpaquetteThu, 08 Sep 2011 21:11:25 GMTcc changedhttps://code.djangoproject.com/ticket/15918#comment:18
https://code.djangoproject.com/ticket/15918#comment:18
<ul>
<li><strong>cc</strong>
<em>martin.paquette@…</em> added
</li>
</ul>
TicketjulienSun, 18 Sep 2011 07:52:40 GMTstatus changed; resolution sethttps://code.djangoproject.com/ticket/15918#comment:19
https://code.djangoproject.com/ticket/15918#comment:19
<ul>
<li><strong>status</strong>
changed from <em>reopened</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
Thanks, this was rectified in <a class="changeset" href="https://code.djangoproject.com/changeset/16850/">r16850</a>.
</p>
Ticket