Skip to content
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

SQL template with triple quote in completion #44646

Closed
wants to merge 12 commits into from
Closed

SQL template with triple quote in completion #44646

wants to merge 12 commits into from

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Sep 3, 2019 •
edited
Loading

Summary

See this issue for more more information.

The idea is to give a slightly improved SQL query experience from console with users often running into issues with needing to use " in their SQL queries.

Also adds format param completion.

cc @costin

Demo of new autocomplete with format toggling

sql_template

Triple quote expansion

triple expands

[Update] including fix for this issue after all

How to review

  • Test that the new SQL autocomplete works as expected (under the GET _sql endpoint).
  • Check that formatting works as expected for """ and non """ request bodies.

@jloleysens jloleysens added enhancement New value added to drive a business result Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.5.0 v8.0.0 labels Sep 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@costin
Copy link
Member

costin commented Sep 5, 2019

Likely this PR needs a reviewer for it to be merged, maybe @cjcenizal ?

@jloleysens jloleysens requested a review from cjcenizal September 5, 2019 08:34
@alisonelizabeth alisonelizabeth self-requested a review September 5, 2019 17:22
@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens jloleysens mentioned this pull request Sep 6, 2019
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

alisonelizabeth
alisonelizabeth reviewed Sep 6, 2019
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

@jloleysens thanks for adding this enhancement!

Tested locally - the autocomplete for sql works great! However, I'm not sure #44729 is resolved. I may be misunderstanding the issue, but I'm still seeing triple quotes converted when I auto-indent.

TRSktNKnNx

@jloleysens
Copy link
Contributor Author

@alisonelizabeth Yeah I think this one is slightly open to interpretation - but I think the issue was describing the previous indentation behaviour:

old-indent-behaviour

Where it should only toggle between two different states instead of the second (incorrect state) of:

GET _sql
{
  "content": " trip\"le "
}

Can you confirm @cjcenizal ?

@jloleysens
Merge branch 'master' of github.com:elastic/kibana into basic_sql_tem…
5d85255 
…plate

* 'master' of github.com:elastic/kibana: (91 commits)
  [APM] Make number of x ticks responsive to the plot width (elastic#44870)
  [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860)
  Replace TSVB timeseries charts with elastic-charts (elastic#33558)
  [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581)
  [alerting] Adds Action Type configuration support and whitelisting (elastic#44483)
  FTR: fix WebDriver Actions calls (elastic#44605)
  [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677)
  [skip-ci][Maps] Improve Maps intro page (elastic#44721)
  [Maps] Update titles and descriptions for data sources (elastic#44833)
  Types + Extract Integration Util (elastic#44433)
  Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933)
  [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359)
  Update Jest script to output coverage (elastic#44447)
  [ftr] support --kibana-install-dir flag (elastic#44552)
  [WATCHER] Allow user to set a threshold value of 0 (elastic#44810)
  Remove injectI18n in dashboard plugin. (elastic#44580)
  [Graph] Save modal (elastic#44261)
  Use external script for the OIDC Implicit flow handler page. (elastic#44866)
  disable router prefixing with pluginId (elastic#44855)
  [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671)
  ...
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cjcenizal
Copy link
Contributor

I haven't had a chance to review this locally, but yes I believe @jloleysens's interpretation was my original understanding too. However... now I'm having second thoughts. Maybe the original behavior was a feature? Is it possible that users want to cycle from expanded with triple quotes -> expanded with escaped quotes -> condensed and back again? Can we confirm this with @bleskes before merging the fix and/or extract the fix into a separate PR to unblock this one?

@bleskes
Copy link
Contributor

bleskes commented Sep 9, 2019

@cjcenizal I'm not sure what I'm supposed to check. #44593 makes sense to me.

cjcenizal
cjcenizal approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested locally, code LGTM! If you decide to extract the bug fix into a separate PR I can re-test this one.

@jloleysens jloleysens requested review from a team as code owners September 10, 2019 10:08
@jloleysens jloleysens requested a review from a team September 10, 2019 10:08
@jloleysens jloleysens requested a review from a team as a code owner September 10, 2019 10:08
@jloleysens
Copy link
Contributor Author

jloleysens commented Sep 10, 2019 •
edited
Loading

Apologies, for the spam - I tried to salvage this by reverting a merge commit and didn't consider the fact that it would create a newer commit undoing a ton of work. Sorry for the spam - please ignore!

I'll just cherry-pick changes and create 2 new, clean PRs.

@jloleysens jloleysens closed this Sep 10, 2019
@jloleysens jloleysens mentioned this pull request Sep 10, 2019
Merged
@jloleysens jloleysens deleted the basic_sql_template branch September 10, 2019 10:40
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth left review comments

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees
No one assigned
Labels
enhancement New value added to drive a business result Feature:Console Dev Tools Console Feature release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jloleysens @elasticmachine @costin @cjcenizal @bleskes @alisonelizabeth