-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ES|QL] Prettiffying the query #191269
[ES|QL] Prettiffying the query #191269
Conversation
@elasticmachine merge upstream |
Pinging @elastic/kibana-esql (Team:ESQL) |
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.
LGTM
const trimmedCode = query.trim().replaceAll('\n |', '\n|'); | ||
const numberOfCommands = ast.length; | ||
const pipesWithNewLine = trimmedCode?.split('\n|'); |
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.
Not sure, but it looks the replaceAll()
might not be necessary:
const trimmedCode = query.trim().replaceAll('\n |', '\n|'); | |
const numberOfCommands = ast.length; | |
const pipesWithNewLine = trimmedCode?.split('\n|'); | |
const trimmedCode = query.trim(); | |
const numberOfCommands = ast.length; | |
const pipesWithNewLine = trimmedCode?.split('\n |'); |
Maybe trim()
as well:
const trimmedCode = query.trim().replaceAll('\n |', '\n|'); | |
const numberOfCommands = ast.length; | |
const pipesWithNewLine = trimmedCode?.split('\n|'); | |
const numberOfCommands = ast.length; | |
const pipesWithNewLine = query.split('\n |'); |
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.
True, adjusted here a881caa
/ci |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
## Summary The wrap by pipes CTA doesnt work correctly when pipes are being used inside functions or in a comment in an ES|QL query. This PR fixes this bug by using the prettifying api. I was thinking to change the tooltip text to mention that it doesnt only wrap by pipes but also prettifies but I decided against it and kept the tooltip as it is for 2 reasons: - this is the most important thing that it does - prettifying means nothing for the majority of our users ![meow](https://github.com/user-attachments/assets/c2189218-4367-4d4a-9f9e-2d6f88f731d2) ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
The wrap by pipes CTA doesnt work correctly when pipes are being used inside functions or in a comment in an ES|QL query.
This PR fixes this bug by using the prettifying api. I was thinking to change the tooltip text to mention that it doesnt only wrap by pipes but also prettifies but I decided against it and kept the tooltip as it is for 2 reasons:
Checklist