-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary #260618
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
Changes from 9 commits
a315d3c
8fa03b5
e20190c
ac38612
c1723af
ff555da
c2b2335
42fc7a6
95dbbf1
5b2e4fd
f05be4b
2cd0a4a
a9a1376
02fe816
a819bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,12 @@ export async function suggestInFunction(ctx: ExpressionContext): Promise<ISugges | |
| } = ctx; | ||
|
|
||
| const functionExpression = expressionRoot as ESQLFunction; | ||
| const paramContext = buildExpressionFunctionParameterContext(functionExpression, context); | ||
| const startingNewParam = STARTING_NEW_PARAM_REGEX.test(innerText); | ||
|
Contributor
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 regex is duplicated. in_function.ts is STARTING_NEW_PARAM_REGEX and stats/autocomplete.ts uses inline /,\s*$/ We could create a shared utility.
Contributor
Author
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. |
||
| const paramContext = buildExpressionFunctionParameterContext( | ||
| functionExpression, | ||
| context, | ||
| startingNewParam | ||
| ); | ||
|
|
||
| if (!paramContext) { | ||
| return []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ import type { | |
| import { suggestForExpression } from '../../definitions/utils'; | ||
| import type { MapParameters } from '../../definitions/utils/autocomplete/map_expression'; | ||
| import { getCommandMapExpressionSuggestions } from '../../definitions/utils/autocomplete/map_expression'; | ||
| import { EDITOR_MARKER } from '../../definitions/constants'; | ||
| import { | ||
| pipeCompleteItem, | ||
| assignCompletionItem, | ||
|
|
@@ -85,7 +84,7 @@ function getPosition( | |
| return { position: CompletionPosition.AFTER_COMMAND }; | ||
| } | ||
|
|
||
| const expressionRoot = prompt?.text !== EDITOR_MARKER ? prompt : undefined; | ||
|
Contributor
Author
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. I tested this by hand, I found no side effects |
||
| const expressionRoot = prompt; | ||
|
|
||
| // (function, literal, or existing column) - handle as primaryExpression | ||
| if (isFunctionExpression(expressionRoot) || isLiteral(prompt) || isExistingColumn) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ import { | |
| noneValueCompleteItem, | ||
| } from '../complete_items'; | ||
| import { withAutoSuggest } from '../../definitions/utils/autocomplete/helpers'; | ||
| import { EDITOR_MARKER } from '../../definitions/constants'; | ||
| import { ESQL_STRING_TYPES } from '../../definitions/types'; | ||
| import { columnExists, findFinalWord } from '../../definitions/utils/autocomplete/helpers'; | ||
| import type { ICommandCallbacks } from '../types'; | ||
|
|
@@ -40,6 +39,9 @@ enum FusePosition { | |
| WITH = 'with', | ||
| } | ||
|
|
||
| // After `KEY BY field,`, suggest the next KEY BY field. | ||
| const KEY_BY_TRAILING_COMMA_REGEX = /\bkey\s+by(?:\s+\S+,)+\s*$/i; | ||
|
|
||
|
Contributor
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. @bartoval let's fix that otherwise when we merge an issue will be created. Let's fix it here
Contributor
Author
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. I had already fixed it here 42fc7a6 that comment refers to the first version
Contributor
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. ah nice |
||
| function getPosition(innerText: string, command: ESQLAstFuseCommand): FusePosition { | ||
| const { scoreBy, keyBy, groupBy, withOption } = extractFuseArgs(command); | ||
|
|
||
|
|
@@ -54,7 +56,7 @@ function getPosition(innerText: string, command: ESQLAstFuseCommand): FusePositi | |
| if ( | ||
| (keyBy && keyBy.incomplete) || | ||
| immediatelyAfterOptionFieldsList(innerText, 'key by') || | ||
| keyBy?.text.includes(EDITOR_MARKER) | ||
| KEY_BY_TRAILING_COMMA_REGEX.test(innerText) | ||
| ) { | ||
| return FusePosition.KEY_BY; | ||
| } | ||
|
|
||
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.
Here the missing part of the task