I recently came across the issue described in #5903 [1] earlier. There are two distinct patches that fix the problem, but at different levels. My inclination is to fix this issue at the model field level and properly override get_default. My feeling is that allowing Decimal objects to pass through force_unicode (when strings_only=True) might cause ill-effects in other parts of Django, but I am not entirely sure (running the test suite with the fix in force_unicode didn't cause any failed test, but that doesn't make it right to me :). I don't see much reason to do so. Perhaps someone can shed some light on this?
On Tue, Feb 10, 2009 at 7:06 PM, Brian Rosner <bros...@gmail.com> wrote:
> Hey all,
> I recently came across the issue described in #5903 [1] earlier. There
> are two distinct patches that fix the problem, but at different
> levels. My inclination is to fix this issue at the model field level
> and properly override get_default. My feeling is that allowing Decimal
> objects to pass through force_unicode (when strings_only=True) might
> cause ill-effects in other parts of Django, but I am not entirely sure
> (running the test suite with the fix in force_unicode didn't cause any
> failed test, but that doesn't make it right to me :). I don't see much
> reason to do so. Perhaps someone can shed some light on this?
On Tue, Feb 10, 2009 at 7:10 PM, Alex Gaynor <alex.gay...@gmail.com> wrote:
> On Tue, Feb 10, 2009 at 7:06 PM, Brian Rosner <bros...@gmail.com> wrote:
>> Hey all,
>> I recently came across the issue described in #5903 [1] earlier. There
>> are two distinct patches that fix the problem, but at different
>> levels. My inclination is to fix this issue at the model field level
>> and properly override get_default. My feeling is that allowing Decimal
>> objects to pass through force_unicode (when strings_only=True) might
>> cause ill-effects in other parts of Django, but I am not entirely sure
>> (running the test suite with the fix in force_unicode didn't cause any
>> failed test, but that doesn't make it right to me :). I don't see much
>> reason to do so. Perhaps someone can shed some light on this?
> It seems force_unicode is the wrong level to fix this at, and doing it in
> get_default() is the right place.
> Alex
> --
> "I disapprove of what you say, but I will defend to the death your right to
> say it." --Voltaire
> "The people's good is the highest law."--Cicero
I sent that last email off rather hastily, and didn't think about it enough,
and of course I've changed my mind :). It seems to me this is the exact
purpose of the strings_only kwarg, for when what we want to do is make sure
a string becomes unicode, but not to mess with other objects. Further it's
clearly how other fields(integer, date, etc.) do it, so that further the
argument that that is the correct way to handle it/use strings_only.
Alex
-- "I disapprove of what you say, but I will defend to the death your right to
say it." --Voltaire
"The people's good is the highest law."--Cicero
On Wed, Feb 11, 2009 at 9:06 AM, Brian Rosner <bros...@gmail.com> wrote:
> Hey all,
> I recently came across the issue described in #5903 [1] earlier. There > are two distinct patches that fix the problem, but at different > levels. My inclination is to fix this issue at the model field level > and properly override get_default. My feeling is that allowing Decimal > objects to pass through force_unicode (when strings_only=True) might > cause ill-effects in other parts of Django, but I am not entirely sure > (running the test suite with the fix in force_unicode didn't cause any > failed test, but that doesn't make it right to me :). I don't see much > reason to do so. Perhaps someone can shed some light on this?
Our usage of Decimals has historically been undertested, so the fact that you get no test failures doesn't necessarily mean that changing force_unicode won't cause problems :-)
However, in this case, I'm reasonably convinced it is the right thing to do. The list of 'ignored' types for force_unicode is essentially the list of data types that we use for native data representations. I can't think of any reason that Decimals shouldn't be on that list.
If you're looking for a little more background on exactly what force_unicode "should" do, here's a discussion from way back, when we expanded the non-string types to include dates, times, etc:
My reading of that (then, as now), is that force_unicode(strings_only=True) exists to catch string proxy objects, not non-string objects; adding Decimal to the list of non-string objects should be fine.
However, before you check anything in, I would suggest making sure that we have got good test coverage for this change. In particular, I would check that we have good Decimal tests for the following:
* Populating initial values on forms. One of the few places that might be relying on get_default() returning a string is in an initial value for a form
* Serialization, especially of an object with a decimal default value.
On Wed, 2009-02-11 at 22:50 +0900, Russell Keith-Magee wrote:
[...]
> However, in this case, I'm reasonably convinced it is the right thing > to do. The list of 'ignored' types for force_unicode is essentially > the list of data types that we use for native data representations. I > can't think of any reason that Decimals shouldn't be on that list.
> If you're looking for a little more background on exactly what > force_unicode "should" do, here's a discussion from way back, when we > expanded the non-string types to include dates, times, etc:
> My reading of that (then, as now), is that > force_unicode(strings_only=True) exists to catch string proxy objects, > not non-string objects; adding Decimal to the list of non-string > objects should be fine.
Yes, that's the idea. I agree that allowing Decimal objects to pass-through natively should be fine.
/me makes a note to add that summary to internal documentation in that future when I have time to write some.
