Merge lp://staging/~cjwatson/storm/psycopg-2.5 into lp://staging/storm

Proposed by Colin Watson
Status: Merged
Approved by: Björn Tillenius
Approved revision: 478
Merged at revision: 478
Proposed branch: lp://staging/~cjwatson/storm/psycopg-2.5
Merge into: lp://staging/storm
Diff against target: 29 lines (+9/-3)
1 file modified
storm/exceptions.py (+9/-3)
To merge this branch: bzr merge lp://staging/~cjwatson/storm/psycopg-2.5
Reviewer Review Type Date Requested Status
Björn Tillenius (community) Approve
Adam Collard (community) Approve
Review via email: mp+278330@code.staging.launchpad.net

Commit message

Use ABCMeta for exception base class injection

This requires Python >= 2.6 (which is unlikely to be a problem these days!),
and adds compatibility with psycopg2 >= 2.5.

Description of the change

Use ABCMeta for exception base class injection.

This lets us be compatible with psycopg2 >= 2.5, where Error and subclasses are implemented in a C extension so can't have their __bases__ attribute patched.

It does introduce a potential obstacle to porting to Python 3 due to a Python bug noted in a comment. Other possible approaches include converting StormError et al into tuples (but that would be an API break if anyone was using them for any purpose other than isinstance/issubclass/exception-handling) or re-raising all database exceptions as Storm exceptions (rather intrusive and possibly error-prone). Since Storm hasn't yet been ported to Python 3 anyway, this doesn't seem like a showstopper, and I think using ABCMeta is a pragmatic approach for the time being.

To post a comment you must log in.
Revision history for this message
Adam Collard (adam-collard) wrote :

Looks good to me! +1

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

Looks good to me as well, +1!

I've confirmed both Landscape and Storm tests pass with this branch and python-psycopg2 2.6.1-1build1.

There are some django tests that fail in Storm, but they fail in trunk as well for me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: