-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use sqlalchemy's 'quote' function to quote table names and fix table quoting in cfr exporter #172
Conversation
Moving back to draft as there seem to be issues. |
It might be totally unrelated, but it also might be related to unfortunate side-effects of shortcomings within the test suite, as only three test cases are failing. Let me bring in this patch first, and then have another look after rebasing. |
Yes I think it's unrelated. This PR should not have issues anymore now. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
===========================================
- Coverage 80.71% 68.33% -12.38%
===========================================
Files 68 68
Lines 2712 2716 +4
===========================================
- Hits 2189 1856 -333
- Misses 523 860 +337
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thank you!
tests/util/database.py
Outdated
assert database.quote_ident("my_schema.my_table") == "my_schema.my_table" | ||
assert database.quote_ident("my-schema.my_table") == '"my-schema".my_table' |
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.
What about this case and friends? a) Does it work? b) Does it deserve a dedicated test case?
assert database.quote_ident('"my_schema"."my_table"') == "my_schema.my_table"
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.
The current logic will not remove such valid quotes, why should it?
Unrelated to this patch specifically, but absolutely related to the same topic, see also that upcoming patch for the SQLAlchemy dialect implementation. We will be happy to have your voice on the relevant discussion, when applicable. |
I think we should rename the |
@amotl |
See commits.
Closes #168.