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
Switch to column keyword arguments #161
Switch to column keyword arguments #161
Conversation
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.
I haven't deeply absorbed the 1.3.0 changes yet, so may have more comments, but this looks reasonable at first read-through.
sqlalchemy_redshift/dialect.py
Outdated
@@ -26,6 +26,7 @@ | |||
pass | |||
else: | |||
from alembic.ddl.base import RenameTable | |||
|
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.
Can you revert whitespace changes or refactor them to a separate PR?
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.
I can, but for some reason this particular whitespace was causing travis build to fail
sqlalchemy_redshift/dialect.py
Outdated
@@ -254,7 +255,9 @@ class RedshiftDDLCompiler(PGDDLCompiler): | |||
... metadata, | |||
... sa.Column('id', sa.Integer, primary_key=True), | |||
... sa.Column( | |||
... 'name', sa.String, info={'distkey': True, 'sortkey': True} | |||
... 'name', sa.String, |
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.
Can we either split these to separate lines, or roll the last two options onto this same line for better consistency?
@ms32035 - If you make the whitespace and docs changes here as commented, I'll kick off CI that hits Redshift and we should be good to merge if it's all passing. |
Co-Authored-By: ms32035 <ms32035@gmail.com>
Co-Authored-By: ms32035 <ms32035@gmail.com>
looks great, feel free to add me as reviewers if you have another one of these in the future |
info
to dialect specific kwargs, which are now supported in SQLAlchemy>=1.3.0, as per recommendation from the author: https://bitbucket.org/zzzeek/alembic/pull-requests/86/add-support-for-column-info/diffinfo
support for SQLAlchemy < 1.3.0 and minimal SQLAlchemy version retainedTodos