-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix SQLAlchemy 1.2.0 compatibility #412
Conversation
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
==========================================
- Coverage 93.53% 93.52% -0.01%
==========================================
Files 25 25
Lines 3648 3647 -1
Branches 194 194
==========================================
- Hits 3412 3411 -1
Misses 198 198
Partials 38 38
Continue to review full report at Codecov.
|
Does the patch work with SQLAlchemy 1.1? |
@asvetlov tests are passing with version 1.1, but as you can see in commit zzzeek/sqlalchemy@31f80b9 SQLAlchemy bumped minimum required version of |
aiopg/sa/result.py
Outdated
@@ -121,9 +120,7 @@ def __init__(self, result_proxy, metadata): | |||
name, obj, type_ = ( | |||
colname, | |||
None, | |||
result_map.get( | |||
colname, | |||
typemap.get(coltype, sqltypes.NULLTYPE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do hasattr(dialect, 'dbapi_type_map')
check, and make code working with both version of sqlalchemy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want to keep dbapi_type_map
compatibility, I suggest something like:
typemap = getattr(dialect, 'dbapi_type_map', {})
and keep result_map
as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jettify if you prefer this way I can fix my changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please also add comment explaining why we do that hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please review
* Update sqlalchemy from 1.1.15 to 1.2.0 * Update aiohttp from 2.3.5 to 2.3.7 * Update raven from 6.3.0 to 6.4.0 * Update pytest from 3.3.0 to 3.3.1 * Update pytest-aiohttp from 0.2.0 to 0.3.0 * rollback SQLAlchemy, ref aio-libs/aiopg#412 * tweaks and fixes while deploying to beta
LGTM, not sure if we we want to test aiopg with both versions of SQLA, change looks trivial. Anyway I will create ticket for to drop this hack and bump minimal required version for SQLA, after one or two releases, this will give some buffer time for upgrade. |
@jettify could you please upload new version of |
already deployed https://pypi.python.org/pypi/aiopg/0.13.2 |
Thanks! |
Just noticed other issue: psycopg/psycopg2#594 we need to bump minimal version of psycopg2, as result half of our builds fail due to this bug. |
Fixes #411
Related SQLAlchemy commit zzzeek/sqlalchemy@31f80b9