Feedback on ticket 7777

[thebitguru]

Hi,

Can I please get some feedback on this ticket? I am hoping that we
can get this in soon.

http://code.djangoproject.com/ticket/7777

Thanks,
Farhan

[thebitguru]

Anyone?

[Alex Gaynor]

> --
>
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
>

Please be patient. At least in the US it's thanksgiving time so I
imagine most of the American committers are busy, plus this list has
been pretty low traffic the last few days, no one is ignoring your
message, people are just busy.

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
"Code can always be simpler than you think, but never as simple as you
want" -- Me

[Russell Keith-Magee]

Echoing Alex's comment - responding to this was on my todo list, but
I've been fighting a head cold for the last few days. On top of that -
we're in the feature development phase, so non-critical bug fixes
aren't taking the highest priority at the moment.

That said: the patch itself looks like a fine implementation of a
given behaviour.

What isn't clear is that the behavior implemented is correct. Malcolm
flagged this in his original comments - NaN and Inf are CPU and
database-dependent values. Postgres can't support Inf, but can support
NaN; I don't know what the situation is with other databases. Inf and
NaN are both Decimal values - unconditionally raising a validation
error doesn't seem like the right approach to me.

So - the patch is fine, as long as we accept the design it implements.
Now you need to convince us that the design is correct.

Yours,
Russ Magee %-)

[thebitguru]

Sorry, I submitted that patch about a month ago and that was the date
stuck in my head :) I just realized that I made my original post only
two days ago. I apologize.

On Nov 25, 12:29 pm, Alex Gaynor <alex.gay...@gmail.com> wrote:
> On Wed, Nov 25, 2009 at 12:15 PM, thebitguru <fah...@gmail.com> wrote:
> > Anyone?
>
> > On Nov 23, 6:38 pm, thebitguru <fah...@gmail.com> wrote:
> >> Hi,
>
> >> Can I please get some feedback on this ticket?  I am hoping that we
> >> can get this in soon.
>
> >>http://code.djangoproject.com/ticket/7777
>
> >> Thanks,
> >> Farhan
>
> > --
>
> > You received this message because you are subscribed to the Google Groups "Django developers" group.
> > To post to this group, send email to django-d...@googlegroups.com.
> > To unsubscribe from this group, send email to django-develop...@googlegroups.com.

> > For more options, visit this group athttp://groups.google.com/group/django-developers?hl=en.

[thebitguru]

Thanks for the reply, Russ.

I need to revise the patch, but I need some confirmation. I am not
sure if it is worth determining what all the different databases
support. I think optional parameters (allow_{inf,nan}=False) that let
the user specify that django should allow these two cases would be
sufficient. Note that I am suggesting setting these to False, which
will break compatibility, but I believe that this is really what the
average user is expecting anyways.

If I can get a confirmation that this behavior will be acceptable then
I will go ahead and submit an updated patch that reflects this
change. This time I will ping again in one week :)

Enjoy the thanksgiving,
Farhan

On Nov 25, 5:56 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:

[Russell Keith-Magee]

On Thu, Nov 26, 2009 at 1:52 PM, thebitguru <fah...@gmail.com> wrote:
> Thanks for the reply, Russ.
>
> I need to revise the patch, but I need some confirmation.  I am not
> sure if it is worth determining what all the different databases
> support. I think optional parameters (allow_{inf,nan}=False) that let
> the user specify that django should allow these two cases would be
> sufficient.  Note that I am suggesting setting these to False, which
> will break compatibility, but I believe that this is really what the
> average user is expecting anyways.

How could it break compatibility? From my testing (i.e., running your
test cases), passing in NaN or Inf raises exceptions at the moment. At
this point, it's impossible for any patch dealing with Inf and NaN to
break backwards compatibility, because there is no working baseline
behavior.

> If I can get a confirmation that this behavior will be acceptable then
> I will go ahead and submit an updated patch that reflects this
> change.  This time I will ping again in one week :)

Adding a configuration doesn't always solve a problem. More often than
not, it adds a new problem. This is one of those cases.

Putting a choice in the hands of a user which can come back and bite
them is not in the style of Django. Defining a DecimalField with
allow_inf=True will cause errors if you use a Postgres database. I'm
not about to commit code that will allow you to have a valid Django
configuration that throws errors in production.

This is why a survey of the current level of support in databases is
important. If, for example, no database supports Inf, then there is no
reason to even have an allow_inf setting. If support is varied, then
we need to have a discussion about the right way to handle the
variation.

There is also the issue of good API design to consider. From a
practical standpoint, I'm having difficulty thinking of a reason why
allow_nan needs to exist - why should an end user be allowed to submit
as a form value that is, a priori, known to be invalid? Essentially,
you're allowing the user to submit "divide by zero" as a valid form
value - what is the use case for this?

In summary, the process goes like this:
1. Get the API right
2. Implement it.

We're still on step 1. :-)

> Enjoy the thanksgiving,

Thanks, but FYI: Thanksgiving on the last Thursday in November is a
US-only holiday. It's held on other dates in some countries, and not
at all in others. Australia doesn't celebrate Thanksgiving at all.

Yours,
Russ Magee %-)

[thebitguru]

OK, you have me convinced :) My assumption was that the developer
changing these options would take the time to ensure that the database
supports it, but I can see why one might assume that if the API
supports it then the database probably does too. I will do a survey
of the django supported databases to see which support the two cases.

I hope you feel better (from the cold, that is :)).

- Farhan

On Nov 26, 12:09 am, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:

[thebitguru]

OK, here is what I have gathered about the databases listed under
DATABASE_ENGINE [4].

Postgresql 8: Supports all three, +/-Inf and NaN [0]
MySQL: No support for either NaN or Inf [1]
sqlite3: No support for either NaN or Inf [2]
Oracle: Supports all three [3]

So, we have a 50/50 split. What do you say?

- Farhan


[0] http://www.postgresql.org/docs/8.2/static/datatype-numeric.html
[1] http://dev.mysql.com/doc/refman/5.4/en/numeric-types.html
[2] http://www.sqlite.org/datatype3.html
[3] http://dev.mysql.com/doc/refman/5.4/en/numeric-types.html
[4] http://docs.djangoproject.com/en/dev/ref/settings/#database-engine

[Russell Keith-Magee]

On Tue, Dec 8, 2009 at 10:18 AM, thebitguru <fah...@gmail.com> wrote:
> OK, here is what I have gathered about the databases listed under
> DATABASE_ENGINE [4].
>
> Postgresql 8: Supports all three, +/-Inf and NaN [0]
> MySQL: No support for either NaN or Inf [1]
> sqlite3: No support for either NaN or Inf [2]
> Oracle: Supports all three [3]
>
> So, we have a 50/50 split.  What do you say?

Combine the complete absence of support in SQLite and MySQL with my
previous comment about the nonsensical nature of NaN and Inf as user
inputs, and my gut reaction is that NaN and Inf should be rejected as
invalid inputs. I'm willing to be convinced otherwise, though.

Yours,
Russ Magee %-)

[Farhan Ahmad]

The existing patch and tests are coded to behave like this so those should be good.  I added our discussion to the ticket for historical purposes.  I don't have to do anything more from here on so I will wait for the final decision.

Thanks,

Farhan

Yours,
Russ Magee %-)

[thebitguru]

Can we please get a final decision on this please?

- Farhan

On Dec 9 2009, 8:17 pm, Farhan Ahmad <fah...@gmail.com> wrote:
> The existing patch and tests are coded to behave like this so those should
> be good.  I added our discussion to the ticket for historical purposes.  I
> don't have to do anything more from here on so I will wait for the final
> decision.
>
> Thanks,
> Farhan
>
> On Wed, Dec 9, 2009 at 4:41 AM, Russell Keith-Magee

> <freakboy3...@gmail.com>wrote:

>
>
>
> > On Tue, Dec 8, 2009 at 10:18 AM, thebitguru <fah...@gmail.com> wrote:
> > > OK, here is what I have gathered about the databases listed under
> > > DATABASE_ENGINE [4].
>
> > > Postgresql 8: Supports all three, +/-Inf and NaN [0]
> > > MySQL: No support for either NaN or Inf [1]
> > > sqlite3: No support for either NaN or Inf [2]
> > > Oracle: Supports all three [3]
>
> > > So, we have a 50/50 split.  What do you say?
>
> > Combine the complete absence of support in SQLite and MySQL with my
> > previous comment about the nonsensical nature of NaN and Inf as user
> > inputs, and my gut reaction is that NaN and Inf should be rejected as
> > invalid inputs. I'm willing to be convinced otherwise, though.
>
> > Yours,
> > Russ Magee %-)
>
> > --
>
> > You received this message because you are subscribed to the Google Groups
> > "Django developers" group.
> > To post to this group, send email to django-d...@googlegroups.com.
> > To unsubscribe from this group, send email to

> > django-develop...@googlegroups.com<django-developers%2Bunsubscr i...@googlegroups.com>

[Russell Keith-Magee]

On Tue, Jan 12, 2010 at 12:05 PM, thebitguru <fah...@gmail.com> wrote:
> Can we please get a final decision on this please?

I was under the impression I *had* given a final decision - we should
reject Inf and NaN as invalid values.

The ticket hasn't been closed because we're not focussing on bug fixes
at the moment - we're still in feature development. Once the feature
freeze happens, we'll move onto fixing bugs.

On an administrative matter - if you want to improve the chances that
this ticket will be closed in the 1.2 bug fixing period, then you
should flag it for the 1.2 milestone.

Yours
Russ Magee %-)

[thebitguru]

Oh, I thought someone else might also have to agree, but anyways, I
see (http://code.djangoproject.com/wiki/Version1.2Roadmap) that the
complete feature freeze is January 26 so I will check again on
February 11 :)

I changed the version to 1.2alpha.

Thanks,
Farhan

On Jan 12, 4:50 am, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:

[Russell Keith-Magee]

On Tue, Jan 12, 2010 at 11:17 PM, thebitguru <fah...@gmail.com> wrote:
> Oh, I thought someone else might also have to agree, but anyways, I
> see (http://code.djangoproject.com/wiki/Version1.2Roadmap) that the
> complete feature freeze is January 26 so I will check again on
> February 11 :)
>
> I changed the version to 1.2alpha.

I didn't say *version* -

>> On an administrative matter - if you want to improve the chances that
>> this ticket will be closed in the 1.2 bug fixing period, then you
>> should flag it for the 1.2 milestone.

I said *milestone*. I've corrected the ticket.

Yours,
Russ Magee %-)