Merge lp:~jamesh/storm/django-support into lp:storm

Proposed by James Henstridge on 2008-07-31
Status: Merged
Merge reported by: James Henstridge
Merged at revision: 268
Proposed branch: lp:~jamesh/storm/django-support
Merge into: lp:storm
Prerequisite: lp:~jamesh/storm/require-less-zope
To merge this branch: bzr merge lp:~jamesh/storm/django-support
Reviewer Review Type Date Requested Status
Jamu Kakar (community) Approve on 2008-08-29
Gustavo Niemeyer Approve on 2008-08-15
Review via email: mp+603@code.launchpad.net
To post a comment you must log in.
Gustavo Niemeyer (niemeyer) wrote :

Looks great! +1!

review: Approve
Gustavo Niemeyer (niemeyer) wrote :

Argh!! That review was for a different merge proposal.

review: Abstain
Gustavo Niemeyer (niemeyer) wrote :

Well, it actually does look good too. :-)

Just a few hints about Mocker use, which may be safely ignored if you don't want to change these now:

[1]

+ self.expect(begin()).result(None)
(...)
+ self.expect(abort()).result(None)

The expect+result isn't strictly necessary in these cases. Just doing

    begin()
    self.mocker.replay()

for instance, would work.

[2]

+ # We test three request methods
+ self.expect(commit()).result(None)
+ self.expect(commit()).result(None)
+ self.expect(commit()).result(None)

Same thing here. This might be written as:

    self.expect(commit()).count(3)

review: Approve
Jamu Kakar (jkakar) wrote :

[1]

+__all__ = ["get_store"]

"get_store_uri" should be defined here too, in storm/django/stores.py.

[2]

+ def tearDown(self):
+ transaction.abort()
+ self.drop_tables()
+ settings._target = None
+ global_zstorm._reset()
+ stores.have_configured_stores = False
+ transaction.manager.free(transaction.get())
+ super(DjangoBackendTests, self).tearDown()

It might be worth making ZStorm._reset public so that we can access
one less private member here.

This is great. I'm looking forward to using it. +1!

review: Approve
lp:~jamesh/storm/django-support updated on 2008-09-11
270. By James Henstridge on 2008-09-11

Fix __all__ for storm.django.stores

271. By James Henstridge on 2008-09-11

merge from trunk

272. By James Henstridge on 2008-09-11

Add NEWS entry for storm.django.

273. By James Henstridge on 2008-09-11

Remove unused import.

Subscribers

People subscribed via source and target branches

to status/vote changes: