-
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] Auto open suggestions for field lists #190466
Conversation
…stions-for-keep-drop
Blocked on #190465 |
…stions-for-keep-drop
…stions-for-keep-drop
/ci |
Pinging @elastic/kibana-esql (Team:ESQL) |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Gave it a go, felt good, and work as described!
Also, nice finds on those other issues.
'date_period[]', | ||
] as const; | ||
|
||
export type ArrayType = (typeof arrayTypes)[number]; |
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!
@@ -289,7 +298,7 @@ const arrayToSingularMap: Map<FunctionParameterType, FunctionParameterType> = ne | |||
* Given an array type for example `string[]` it will return `string` | |||
*/ | |||
export function extractSingularType(type: FunctionParameterType): FunctionParameterType { | |||
return arrayToSingularMap.get(type) ?? type; | |||
return isArrayType(type) ? arrayToSingularMap.get(type)! : type; |
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.
Looking at this again, will there ever be a case where we can not remove the last []
in the text to get the singular type? Do we need to look up a map for it?
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.
We could always just remove the []
through string manipulation. I like the map because it makes it clear which types can be arrays. I guess this whole array types thing makes me nervous because I don't have a good handle yet on its purpose. So, I like being a bit more explicit.
Open to opinions though.
Tested and LGTM 🎉 |
Summary
Part of #189662
Suggests comma and pipe
Screen.Recording.2024-08-06.at.5.09.58.PM.mov
Doesn't suggest comma when there are no more fields
Screen.Recording.2024-08-22.at.9.35.51.AM.mov
Doesn't work for escaped columns :(
Screen.Recording.2024-08-22.at.9.38.16.AM.mov
As part of this effort I discovered #191100 and #191105, as well as a problem with column name validation (see #177699)
I think we can revisit column escaping and probably resolve all of these issues (issue here).
Checklist