EliAndrewC / sideboard

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

sideboard.lib.sa.declarative_base needs some way to set an initializer #65

Closed EliAndrewC closed 10 years ago

EliAndrewC commented 10 years ago

So let's say you do this:

@declarative_base
class Base:
    def __init__(self, *args, **kwargs):
        ...

class Foo(Base):
    ...

Intuitively you'd expect that calling Foo() would end up invoking Base.__init__ but that's not actually how SQLAlchemy works. Even if we take the complex Sideboard stuff out of the picture and use plain old SQLAlchemy, here's what we get:

import sqlalchemy
from sqlalchemy.schema import Column
from sqlalchemy.types import Integer
from sqlalchemy.ext.declarative import declarative_base

class Base:
    id = Column(Integer, primary_key=True)

    def __init__(self, *args, **kwargs):
        print('this print statement is never reached')

Base = declarative_base(cls=Base, )

class Foo(Base):
    __tablename__ = 'foo'

Foo()
print(Foo.mro())

That last statement tells us that the method resolution order is Foo then sqlalchemy.ext.declarative.api.Base then Base then object. From consulting the docs (http://docs.sqlalchemy.org/en/rel_0_9/orm/extensions/declarative.html#sqlalchemy.ext.declarative.declarative_base), it looks like the solution is to use the constructor parameter.

I propose that sideboard.lib.sa.declarative_base be smart enough to check your declarative base class to see if it defined its own __init__ and then use that as the constructor parameter if it did. (Note that SQLAlchemy bypasses __init__ when constructing objects when reading rows from a database, but there's a documented way to have it run a similar function in that situation as well, but that's orthogonal to this ticket.)

EliAndrewC commented 10 years ago

Note that this is the first of what I expect to be many tickets to come out of converting the MAGFest code from the Django ORM to SQLAlchemy.

EliAndrewC commented 10 years ago

This turns out to be a really simple change, here's the diff:

-    Mixed = declarative.declarative_base(cls=Mixed)
+    constructor = {'constructor': klass.__init__} if '__init__' in klass.__dict__ else {}
+    Mixed = declarative.declarative_base(cls=Mixed, **constructor)

I'm going to wait to make this pull request, since I want to use this for my MAGFest code, so I don't want it relegated to a separate branch. I guess theoretically I could make a separate branch for it and then pull it into my own main branch, or something. If I end up with too many unpushed changes then I'll probably do that.