Request for testing: #5461
Russell Keith-Magee
I think I've pretty much finished work on #5461 - this is a
refactoring of the database backend to make everything class based,
and get all the table creation bits of Django behind the backend. I've
uploaded the latest patch to the ticket. I'd like some feedback before
I push this into trunk.
A quick summary of what the patch does:
1) Moves most of core.management.sql into backend.DatabaseCreation or
backend.DatabaseIntrospection. Anything that outputs SQL has been
moved behind the backend.
2) Removes a lot of DatabaseFeature entries in favor of overriding
methods on subclasses
3) Moves the test database creation tools into DatabaseCreation
4) Moves the various backend helper classes (DatabaseFeatures,
DatabaseIntrospection etc) to be instance attributes, rather than
class attributes, and gives some of the helper classes (Creation and
Introspection) access to the parent DatabaseWrapper.
The patch passes the full test suite for me with SQLite, MySQL** and
Postgres (psycopyg2). However, I don't have access to an Oracle
install, so it's possible that the oracle-specific bits of the patch
are a bit flaky, but if they are, it should be fairly simple fixes
(changing referecnces to connection.ops, etc). Any assistance from the
Oracle-enabled crew would be most welcome.
I'd like to get this into trunk early to mid next week (prior to the
1.0 beta); if you have feedback, now would be a very good time :-)
** The boolean/1/0 issue causes 1 failure on my setup, but that isn't
the result of this patch
Yours,
Russ Magee %-)
Malcolm Tredinnick
it on.
On Sat, 2008-08-09 at 22:55 +0800, Russell Keith-Magee wrote:
[..]
> I'd like to get this into trunk early to mid next week (prior to the
> 1.0 beta); if you have feedback, now would be a very good time :-)
If we're going to do this (and in general I think we should), I'd like
to go in as soon as possible. There are some other beta-level tickets
that are playing in the same space, so you should go first and then I
can run around behind and do the MySQL binary/collation stuff (I was
going to fix that tomorrow, but it can wait a day or two for this). If
we wait until mid-week that leaves only 24 hours for some other changes
that will need testing just to make sure the beta doesn't fall over.
I don't see anything obviously wrong with the patch from having read it
through carefully. I'll run it later and try some things out.
As a style nit, I personally don't like this sort of thing:
from django.db.backends import BaseDatabaseFeatures
from django.db.backends import BaseDatabaseValidation
from django.db.backends import BaseDatabaseWrapper
from django.db.backends import util
since it makes it look like there are a lot more files involved in
imports than there really are (that's only importing stuff from one
namespace). Personally, I'd just import backends and namespace things,
but that doesn't seem to be the Django style (a flaw in the style,
clearly). Otherwise, use the comma-separated form. Hardly a showstopper
and it's certainly your bikeshed, but it's not the way any other code is
written.
Something I noticed reading through (and this is where you were copying
existing code): there's code like this, in the postgresql backend:
class DatabaseValidation(BaseDatabaseValidation):
pass
(that one's new, but DatabaseFeatures in the same file is old and
similar). Any reason why we really need a whole new class declaration
here? Seems neater to just use BaseDatabaseValidation directly, doesn't
it?
> ** The boolean/1/0 issue causes 1 failure on my setup, but that isn't
> the result of this patch
I'm going to change those tests one day to just compare to the right
results rather than printing out the results and comparing the string
output. That will fix the 1/0 boolean issue (since 1 == True is
guaranteed in Python 2). So don't worry about that.
Regards,
Malcolm
Russell Keith-Magee
<mal...@pointy-stick.com> wrote:
>
> This is a huge, not entirely exciting piece of work. Thanks for taking
> it on.
No problems. There's actually a little bit of itch scratching involved
- django-evolution will be able to make good use of the features
exposed by this refactor. Plus, I kind of enjoy refactoring - call me
wierd, but I like the zen-like nature of changing nothing, yet
changing everything.
> On Sat, 2008-08-09 at 22:55 +0800, Russell Keith-Magee wrote:
> [..]
>> I'd like to get this into trunk early to mid next week (prior to the
>> 1.0 beta); if you have feedback, now would be a very good time :-)
>
> If we're going to do this (and in general I think we should), I'd like
> to go in as soon as possible. There are some other beta-level tickets
> that are playing in the same space, so you should go first and then I
> can run around behind and do the MySQL binary/collation stuff (I was
> going to fix that tomorrow, but it can wait a day or two for this). If
> we wait until mid-week that leaves only 24 hours for some other changes
> that will need testing just to make sure the beta doesn't fall over.
>
> I don't see anything obviously wrong with the patch from having read it
> through carefully. I'll run it later and try some things out.
I'm agreed that time is of the essence. That being the case, I'll aim
to check this in Monday evening, my time (GMT+8). That will give you a
chance to take the patch for a ride, plus a few days to shake out any
bugs I've missed (as I'm sure I have in the Oracle backend).
> As a style nit, I personally don't like this sort of thing:
>
> from django.db.backends import BaseDatabaseFeatures
> from django.db.backends import BaseDatabaseValidation
> from django.db.backends import BaseDatabaseWrapper
> from django.db.backends import util
Fair comment. I did this because comma separation was leading to some
monster import lines. Looking at the backends module, everything in
the module is designed to be imported, so I've changed the imports to
the form 'from django.db.backends import *'.
> Something I noticed reading through (and this is where you were copying
> existing code): there's code like this, in the postgresql backend:
>
> class DatabaseValidation(BaseDatabaseValidation):
> pass
>
> (that one's new, but DatabaseFeatures in the same file is old and
> similar). Any reason why we really need a whole new class declaration
> here? Seems neater to just use BaseDatabaseValidation directly, doesn't
> it?
Also fair comment. I used this approach to mirror the code that was
already there, but in retrospect, it's overkill. I've removed the
empty cases.
I've updated the patch (rc3), including a few bits in contrib.gis that
I missed previously. Barring feedback to the contrary, I'll check this
in tomorrow evening my time.
Yours,
Russ Magee %-)