bmuller / twistar

Twistar is an object-relational mapper (ORM) for Python that uses the Twisted library to provide asynchronous DB interaction.
http://findingscience.com/twistar
Other
132 stars 38 forks source link

Revamped polymorphic support #5

Closed xadhoom closed 13 years ago

xadhoom commented 13 years ago

Hi,

following your suggestions I've rewrote the support for polymorphic relations using a way better dict approach.

A couple of things (based on my small experience):

Tests has been updated to be more "real".

If is ok for you, I'll write some doc.

bmuller commented 13 years ago

Hello xadhoom - thanks for your work. I was reviewing your changes and saw a number of things that did not make sense to me (for instance, I don't know why aliasing is necessary). I decided to go ahead and implement the changes necessary for polymorphism - they are now in master. The only changes I made were to the relationships.py file. You can find the docs for the addition of polymorphic support at http://findingscience.com/twistar/doc/relationship_examples.html#auto4 and tests in the test_relationships.py file.

If there is anything I have left out of my implementation that you would like to see, let me know.

Thanks, Brian

xadhoom commented 13 years ago

The full aliasing support is needed to refer to the interface instead of referring to the 'name', like AR does.

In your doc : def setNickname(nickname, boyOrGirl): boyOrGirl.nicknames.set([nickname]).addCallback(lambda _: getPerson(nickname))

should be: def setNickname(nickname, boyOrGirl): boyOrGirl.nicknameable.set([nickname]).addCallback(lambda _: getPerson(nickname))

otherwise makes little sense specifing an interface and then refer to it in two different ways... You use the interface name when referring the relation from the belonged object and refer to the real attribute when referring from the belonger... The beauty of an interface is that hides you from the real attribute name, and you don't even need to know it.

what do you think about ?

xadhoom commented 13 years ago

Ok, looking to AR seems that they do like your implementation does, not like mine. I was plain wrong, sorry.

So can be ok, just to be more "standard". But I liked the full aliasing :)

bmuller commented 13 years ago

In your example, using: boyOrGirl.nicknameable.set([nickname])

doesn't make sense to me, because boyOrGirl is what is nicknameable. Boys and girls can be nicknamed. Therefore, the property you are setting is their nicknames, not a property they have that is nicknameable. In the same way you would use a regular many relationships set - boy.nicknames.set() when you want to set a boy's many nicknames.

It's the other way around - when you have a nickname - that you would want to get whatever is nicknameable - so nickname.nicknameable.get() should return something that is nicknameable - in this case, a boy or a girl.

xadhoom commented 13 years ago

ok,

makes sense.

Last thing, not polymorphic related, is about this commit: https://github.com/xadhoom/twistar/commit/ff0d663d479fdaafcaf1132120146a614b0c83ea

that makes twistar work with unicode strings, otherwise saving "exotic" charset will fail. Can this be added ?

bmuller commented 13 years ago

It was added - see https://github.com/bmuller/twistar/commit/3bab792a3ddf4d97a03446ee9ef16564ea5eb340

Thanks again for contributing. Let me know if there's anything else I can do to make twistar more useful!

B

xadhoom commented 13 years ago

thanks for now!

I'll going on using it and report back! Great work :)