-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(sqllab): Format sql #25344
Merged
Merged
feat(sqllab): Format sql #25344
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
83cea9a
feat(sqllab): Format sql
justinpark b0b6bca
openapi spec
justinpark 53ef737
remove line
justinpark 033e41f
api schema ref and line ordering
justinpark bf9ad0e
remap keyboard shortcut
justinpark f18f7ae
change endpoint pathname
justinpark 49a9da5
fix format_sql for fetchMock
justinpark d341c71
update model.get("sql") by model["sql"]
justinpark 7ef8a51
pylint
justinpark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,8 +146,10 @@ const AceEditorWrapper = ({ | |
}; | ||
|
||
const onChangeText = (text: string) => { | ||
setSql(text); | ||
onChange(text); | ||
if (text !== sql) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change prevents the overwrite the sql by |
||
setSql(text); | ||
onChange(text); | ||
} | ||
}; | ||
|
||
const { data: annotations } = useAnnotations({ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -72,6 +72,7 @@ import { | |||||||||
scheduleQuery, | ||||||||||
setActiveSouthPaneTab, | ||||||||||
updateSavedQuery, | ||||||||||
formatQuery, | ||||||||||
} from 'src/SqlLab/actions/sqlLab'; | ||||||||||
import { | ||||||||||
STATE_TYPE_MAP, | ||||||||||
|
@@ -305,6 +306,10 @@ const SqlEditor: React.FC<Props> = ({ | |||||||||
[ctas, database, defaultQueryLimit, dispatch, queryEditor], | ||||||||||
); | ||||||||||
|
||||||||||
const formatCurrentQuery = useCallback(() => { | ||||||||||
dispatch(formatQuery(queryEditor)); | ||||||||||
}, [dispatch, queryEditor]); | ||||||||||
|
||||||||||
const stopQuery = useCallback(() => { | ||||||||||
if (latestQuery && ['running', 'pending'].indexOf(latestQuery.state) >= 0) { | ||||||||||
dispatch(postStopQuery(latestQuery)); | ||||||||||
|
@@ -384,8 +389,16 @@ const SqlEditor: React.FC<Props> = ({ | |||||||||
}), | ||||||||||
func: stopQuery, | ||||||||||
}, | ||||||||||
{ | ||||||||||
name: 'formatQuery', | ||||||||||
key: KeyboardShortcut.CTRL_SHIFT_F, | ||||||||||
descr: KEY_MAP[KeyboardShortcut.CTRL_SHIFT_F], | ||||||||||
func: () => { | ||||||||||
formatCurrentQuery(); | ||||||||||
}, | ||||||||||
Comment on lines
+396
to
+398
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
}, | ||||||||||
]; | ||||||||||
}, [dispatch, queryEditor.sql, startQuery, stopQuery]); | ||||||||||
}, [dispatch, queryEditor.sql, startQuery, stopQuery, formatCurrentQuery]); | ||||||||||
|
||||||||||
const hotkeys = useMemo(() => { | ||||||||||
// Get all hotkeys including ace editor hotkeys | ||||||||||
|
@@ -602,7 +615,7 @@ const SqlEditor: React.FC<Props> = ({ | |||||||||
? t('Schedule the query periodically') | ||||||||||
: t('You must run the query successfully first'); | ||||||||||
return ( | ||||||||||
<Menu css={{ width: theme.gridUnit * 44 }}> | ||||||||||
<Menu css={{ width: theme.gridUnit * 50 }}> | ||||||||||
<Menu.Item css={{ display: 'flex', justifyContent: 'space-between' }}> | ||||||||||
{' '} | ||||||||||
<span>{t('Autocomplete')}</span>{' '} | ||||||||||
|
@@ -622,6 +635,7 @@ const SqlEditor: React.FC<Props> = ({ | |||||||||
/> | ||||||||||
</Menu.Item> | ||||||||||
)} | ||||||||||
<Menu.Item onClick={formatCurrentQuery}>{t('Format SQL')}</Menu.Item> | ||||||||||
{!isEmpty(scheduledQueriesConf) && ( | ||||||||||
<Menu.Item> | ||||||||||
<ScheduleQueryButton | ||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 couldn't we just us a npm package to do this work vs. creating a whole new endpoint?
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.
@hughhhh We chose server-side formatting over client-side formatting for several reasons:
If you and other community members believe that an NPM package solution is better, I am happy to make changes.
@betodealmeida @eschutho @michael-s-molina do you have an opinion on the frontend approach?
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.
There seems to be a number of pros with having this on the backend including: custom formatting, dialect specific formatting, and consistency throughout the application.
@villebro has experience working with frontend SQL formatting and ran into a number of issues.