8000 fix: use empty schema name by default by IlyaFaer · Pull Request #32 · googleapis/python-spanner-sqlalchemy · GitHub
[go: up one dir, main page]
Skip to content

fix: use empty schema name by default #32

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Apr 6, 2021
Merged

fix: use empty schema name by default #32

merged 14 commits into from
Apr 6, 2021

Conversation

IlyaFaer
Copy link
Contributor
@IlyaFaer IlyaFaer commented Mar 19, 2021 •
edited
Loading

The PR is based on another: #24

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 19, 2021
IlyaFaer
IlyaFaer commented Mar 19, 2021
View reviewed changes
google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py
@@ -515,7 +527,7 @@ def get_table_names(self, connection, schema=None, **kw):
FROM information_schema.tables
WHERE table_schema = '{}'
""".format(
schema
schema or ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default schema arg is None, so it tried to search for tables with schema 'None'. Not sure how I've written this, it's not smart at all. Fixing it.

IlyaFaer
IlyaFaer commented Mar 19, 2021
View reviewed changes
test/test_suite.py
answer = ["users", "user_tmp", "email_addresses", "dingalings"]
eq_(table_names, answer)
else:
answer = ["dingalings", "email_addresses", "user_tmp", "users"]
Copy link
Contributor Author
@IlyaFaer IlyaFaer Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

user_tmp is a temporary table, adding it into the expected results

@IlyaFaer
Copy link
Contributor Author
IlyaFaer commented Mar 19, 2021 •
edited
Loading

@larkee, this PR fixes two more reflection tests. Still, #24 better be merged first.

Безымянный

@IlyaFaer IlyaFaer requested a review from larkee March 19, 2021 10:35
@IlyaFaer IlyaFaer mentioned this pull request Mar 19, 2021
Merged
larkee
larkee reviewed Mar 23, 2021
View reviewed changes
Copy link
Contributor
@larkee larkee left a comment

Choose a reason for hiding this comment

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

Changes introduced in this PR (i.e. schema name and table name test override) LGTM 👍

Approval pending the base PR

@AVaksman
Copy link
Contributor
AVaksman commented Apr 6, 2021

Merging to unblock further work

@AVaksman AVaksman merged commit f2af0b1 into main Apr 6, 2021
@IlyaFaer IlyaFaer deleted the get_table_names branch April 6, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkee larkee larkee left review comments

@AVaksman AVaksman AVaksman approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IlyaFaer @AVaksman @larkee
0