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

Fixed issues #30, #32, #33 and #34 #31

Closed erikkaplun closed 5 years ago

bmuller commented 11 years ago

Thanks for these! I'll merge them in this weekend.

erikkaplun commented 11 years ago

Actually there is a problem: if afterInit is called in __init__, it is hard to catch any deferred exceptions in it.

Also, the call to afterInit must be removed in createInstances—otherwise it gets called twice. (this is how i discovered the exception catching issue.

A workaround would be to override __new__ and make it return a Deferred, which then returns an instance, so that instead of:

 obj = MyObj()

we'd have

 obj = yield MyObj()  # assuming the use of defer.inlineCallbacks

This might even be a nice complement to the idea of a fully ascynhronous ORM—even object instantiation would be asynchronous.

bmuller commented 11 years ago

Hey eallik - these changes look good but they seem to break 28 tests.

erikkaplun commented 11 years ago

One might expect me to have updated the tests as well, I agree, but nevertheless I explicitly stated that the changes are not backwards compatible, so yeah... Can you perhaps look into it and see if there's something trivial to make most of them pass again? I'd look into the non trivial ones. It's most probably the circumstance that constructing an object now in general returns a deferred (where ever afterInit returns one).

erikkaplun commented 11 years ago

...just nevermind my previuos comment—tests pass now :) (except the failure described in Issue #35 for which I've sent you another pull request so you could merge that independently of the changes here).

erikkaplun commented 11 years ago

Oh and I think this should cause a version bump to 1.1 because of the backwards incompatible change in the behavior of afterInit and model object instantiation (if you decide to merge, that is).

erikkaplun commented 11 years ago

Ping?

bmuller commented 11 years ago

I need some time to take a look at this in detail. Your changes break all backwards compatibility, and that's not something I'm going to accept lightly (and therefore not quickly). Hopefully I'll have some time to look at it this weekend or next.

My intent was to always make object instantiation synchronous, and only produce a deferred when there is necessary interaction w/ the DB. This represents a distinct departure from that intention - and I need to take some time to weigh the benefits / consequences.

erikkaplun commented 11 years ago

Actually there is a conceptual/design problem with afterInit still. Basically the same afterInit for both after loading an existing record from the DB and after creating a completely new unsaved instance does not seem to work. E.g. if I want to apply json.loads to a JSON field. But at the same time, there needs to be a place where you do stuff that you normally do in __init__. But the doc says to use afterInit. But afterInit is not currently called by __init__.

So I'd like to propose a better solution. This solution is not backwards compatible because it requires afterInit to be renamed to afterLoad, but at least instance creation is synchronous.

There might be issues with this approach—if you can point out any, I'd be happy.