-
Notifications
You must be signed in to change notification settings - Fork 29
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
Feature add sql formatter #84
base: main
Are you sure you want to change the base?
Conversation
Body: ==== End ====
Body: ==== End ====
Body: ==== End ====
Body: ==== End ====
Body: ==== End ====
UPD ah, my bad, I see what you're doing. Interesting. |
Hi, I opened a PR to pgcli last month (dbcli/pgcli#1366) to export output to sqls like mycli My idea is that:
For sql output, table name is needed, so parsing function is required. Since cli_helpers only does output format job, so I think it's not good enough to import sqlparse pkg here. So any suggestions? Thanks for your attention. 😁 |
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.
Hi @astroshot, sorry it took forever to review this. I think this change can be useful to multiple CLIs, even if it will need some tweaking in the future. I left a couple of small comments, but otherwise this looks nice.
# -*- coding: utf-8 -*- | ||
|
||
supported_formats = ( | ||
"sql-insert", |
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.
These formats will need documenting.
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.
Sure, I'll add some later
delimiter: Character surrounds table name or column name when it conflicts with sql keywords. | ||
For example, mysql uses ` and postgres uses " | ||
""" | ||
extract_table_func = kwargs.get("extract_tables") |
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.
This is one way to do it. Alternatively, the CLI could pass in tables
as part of kwargs
.
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'll try it
if not isinstance(delimiter, str): | ||
delimiter = '"' | ||
|
||
if tables is not None and len(tables) > 0: |
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.
This adapter doesn't really make sense for more than one table, correct? Perhaps there should be an error if we have more than one 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.
Yes, this formatter is convenient is generate INSERTION SQL or UPDATING SQL relates to one table. In those cases when SQL with multiple tables is run, usually queried results earn more concern than considering to importing them. So I thought it was ok to use a fake table name "DUAL" to indicate this situation, and you?
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 think we should raise an exception in this case.
Description
Add sql-formatter for *clis, which convert output data to sql format like insertion sqls or updating sqls.
For example, in pgcli, you can get sql-insert output like this:
Checklist
CHANGELOG
.AUTHORS
file (or it's already there).pip install pre-commit && pre-commit install
), and ranblack
on my code.