Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Revamped polymorphic support #5

Closed
wants to merge 22 commits into from
Closed

Conversation

xadhoom
Copy link
Contributor

@xadhoom xadhoom commented Jan 27, 2011

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):

  • a small modification to DBObject is needed to fully alias return results.
  • changed how Registry works. Now classes are not in a plain dict, but in a more complex structure
    to save additional informations used for alias lookup. Maybe is slower on lookup but saves more infos.
  • then changed the Relationship init

Tests has been updated to be more "real".

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

@bmuller
Copy link
Owner

bmuller commented Jan 31, 2011

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
Copy link
Contributor Author

xadhoom commented Jan 31, 2011

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
Copy link
Contributor Author

xadhoom commented Jan 31, 2011

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
Copy link
Owner

bmuller commented Feb 1, 2011

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
Copy link
Contributor Author

xadhoom commented Feb 1, 2011

ok,

makes sense.

Last thing, not polymorphic related, is about this commit:
VoiSmart@ff0d663

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

@bmuller
Copy link
Owner

bmuller commented Feb 1, 2011

It was added - see 3bab792

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

B

@xadhoom
Copy link
Contributor Author

xadhoom commented Feb 1, 2011

thanks for now!

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

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants