-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] improve tokenizer and theme #190170
Conversation
614e737
to
e924038
Compare
/ci |
1 similar comment
/ci |
/ci |
} | ||
}); | ||
|
||
it('every valid lexical name should have a corresponding rule', () => { |
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.
Do we like this test? It seemed like a good idea as an extra check that we aren't missing some lexical token that gets added. But, we can remove if it seems like a pain.
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.
Yeah, leave it and let's see how painful it gets
/ci |
/ci |
'false', // @TODO consider if this should get styling | ||
'true', // @TODO consider if this should get styling | ||
'info', // @TODO consider if this should get styling | ||
'colon', // @TODO consider if this should get styling |
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.
Does kind of feel like these should be styled somehow cc @ryankeairns
/ci |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
nice!
Summary
Adds some validation around our Monaco theme and tokenizer. I thought fixing the tokenizer would help with #187184 but it looks like providing an explicit replacement range is actually the right route.
Still, we got a few things out of this since these changes fix a few styling issues.
INLINESTATS
Casting operator
Timespan literals
Subsequent nulls last/first
MATCH
Checklist