Skip to content
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

Reflect column comments #186

Merged
merged 2 commits into from Dec 17, 2019
Merged

Reflect column comments #186

merged 2 commits into from Dec 17, 2019

Conversation

eeshugerman
Copy link
Contributor

@eeshugerman eeshugerman commented Dec 14, 2019

Todos

  • MIT compatible
  • Tests
  • Documentation -- N/A I think?
  • Updated CHANGES.rst

@@ -3,6 +3,7 @@

- Expose supported types for import from the dialect
(`Issue #181 <https://github.com/sqlalchemy-redshift/sqlalchemy-redshift/issues/181>`_)
- Reflect column comments
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no GitHub issue... should I create one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking to the PR is fine.

CHANGES.rst Show resolved Hide resolved
kw.pop('comment', None)
if sa.__version__ < '1.2.0':
# SQLAlchemy 1.2.0 introduced the 'comment' param
del kw['comment']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we need to invert this condition here? Is there ever a possibility that 'comment' will not exist in kwargs, and this could raise an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, the only place this method is called is on line ~480 (above), and I've added the comment kwarg there, so this will never raise a KeyError.

It makes sense to invert the condition because otherwise it would be something like

if sa.__version__ >= '1.2.0':
    pass
else:
    del kw['comment']

Co-Authored-By: Jeff Klukas <jeff@klukas.net>
@jklukas jklukas merged commit ab2df16 into sqlalchemy-redshift:master Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants