-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: auto add quotes for table/columns #1109
Conversation
@@ -559,13 +559,11 @@ export const QueryEditor: React.FC< | |||
|
|||
const handleOnFocus = useCallback( | |||
(editor: CodeMirror.Editor, event) => { | |||
autoCompleter.registerHelper(); |
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.
why it's not needed?
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.
actually it is needed, otherwise query hint wouldn't work. I changed back the code and added more comments
@@ -233,7 +257,7 @@ export class SqlAutoCompleter { | |||
(id) => dataSources.dataColumnsById[id].name | |||
); | |||
const filteredColumnNames = columnNames.filter((name) => | |||
name.startsWith(prefix) | |||
name.toLowerCase().startsWith(prefix) |
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.
is prefix
also lowercased?
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, in this line
searchStr = token.string.toLowerCase();
const schemaTableNames = tableName.split('.'); | ||
|
||
if (schemaTableNames.length === 2) { |
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 feel like I saw this similar code piece multiple times, might be good to have tiny util function for it? can return an object with parsed schema name and table name, so that
const {schemaName, tableName} = parseFullTableName(..)
if (schemaName && tableName) {
...
this.flatQuotationFormatter(tableName)
...
}
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.
hmm it is just .split('.')
and used in 2 places, i think we can make a function for it if it is more common in future
} else if ( | ||
lineAnalysis.context === 'column' && | ||
lineAnalysis.reference | ||
) { | ||
results = this.getColumnsFromPrefix( | ||
searchStr, | ||
lineAnalysis.reference | ||
).map(formatter.bind(null, lineAnalysis.context)); | ||
) | ||
.map(this.flatQuotationFormatter) |
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.
how about just move this .map(this.flatQuotationFormatter)
into getColumnsFromPrefix
?
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 moved all of the quotation formatting logic to the formatter level so it would not interfere with display
* feat: auto add quotes for table/columns * fix wrong sql hint being registered * update according to comments * remove autocompleter from hooks ret
* feat: auto add quotes for table/columns * fix wrong sql hint being registered * update according to comments * remove autocompleter from hooks ret
If a column is like "foo/bar" then querybook would auto add quotes around the column names, similarly for tables.
For Presto
For MySQL
also 2 bug fixes: