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

Fix 'Engine' object has no attribute 'connection' #255

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Oct 10, 2022

Description

Fixes #253

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Fix 'Engine' object has no attribute 'connection', when calling `get_table_comment` from Superset with sqlalchemy 1.3 

@cla-bot cla-bot bot added the cla-signed label Oct 10, 2022
@mdesmet
Copy link
Contributor Author

mdesmet commented Oct 10, 2022

This issue seems to occur only in combination with sqlalchemy 1.3. So we better first merge #254

Copy link
Member

@hovaesco hovaesco left a comment

Choose a reason for hiding this comment

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

Please slightly modify the release note entry to:

Fix 'Engine' object has no attribute 'connection', when calling `get_table_comment` from Superset 

@@ -11,6 +11,7 @@
# limitations under the License
import pytest
import sqlalchemy as sqla
from sqlalchemy import inspect
Copy link
Member

Choose a reason for hiding this comment

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

It's already imported in one line above.

metadata.create_all(engine)
insp = inspect(engine)
actual = insp.get_table_comment(table_name='table_with_id', schema="test")
assert actual['text'] is None
Copy link
Member

Choose a reason for hiding this comment

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

can we have a test for some actual comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be great, but it seems to not work to add a comment through sqlalchemy. Let's create a bug for that.

Copy link
Member

Choose a reason for hiding this comment

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

create an issue + add a TODO. Goal is to make sure if someone other than you works on that issue then they can easily find places in code which need updating otherwise they may end up adding duplicated tests.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % TODO + issue about setting comment.

metadata.create_all(engine)
insp = inspect(engine)
actual = insp.get_table_comment(table_name='table_with_id', schema="test")
assert actual['text'] is None
Copy link
Member

Choose a reason for hiding this comment

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

create an issue + add a TODO. Goal is to make sure if someone other than you works on that issue then they can easily find places in code which need updating otherwise they may end up adding duplicated tests.

@mdesmet
Copy link
Contributor Author

mdesmet commented Oct 11, 2022

@hashhar : Added TODO and created follow up issue for comment support in sqlalchemy. See #256

@hashhar hashhar merged commit ce60a48 into trinodb:master Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

'Engine' object has no attribute 'connection' when calling get_table_comment
3 participants