-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Expressions] Add support of comments #122457
Conversation
aa94013
to
2f14160
Compare
Pinging @elastic/kibana-app-services (Team:AppServicesSv) |
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.
VisEditors changes LGTM, code review only - simple type rename
@elasticmachine merge upstream |
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
}; | ||
|
||
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions | ||
export type AstFunction = { |
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.
i think we should try to get rid of duplicate types for this (ones inside expressions plugin and ones inside kbn-interpreter package. If we add optional debug property on this one i think we can reuse this ones in expressions plugin. not necesarry part of this pr.
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.
That's what I tried already. I did that with type augmentation in the expression plugin as the cleanest solution, but that's causing so many type errors with the PersistableState
generics as they require serializable objects. Fixing those errors caused many changes in other parts of the code using the expressions plugin.
I don't think it would be correct to declare the debug
property in the interpreter package because it doesn't belong there. It's a custom logic built on top of the tree structure in the expressions plugin.
In the end, I refactored those types in the expressions plugin to extend the related ones from the interpreter. As a result, I removed all the duplicating typings and kept only the ones produced by the expressions plugin. This approach also keeps our public API unchanged.
@@ -36,7 +36,7 @@ const collectArgs = (args: ExpressionFunctionAST['arguments']) => { | |||
); | |||
}; | |||
|
|||
export function adaptCanvasFilter(filter: ExpressionFunctionAST): Filter { | |||
export function adaptCanvasFilter(filter: AstFunction): Filter { |
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.
nit: i preferred the old names ExpressionFunctionAST
over just AstFunction
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.
I had a few points to rename them.
- They were inconsistent
Ast
versusExpressionFunctionAST
andExpressionArgAST
(Ast
vsAST
, and notExpressionAST
for some reason). ExpressionArgAST
- shortcut in the name is not a good practice.- The
AST
context was in the suffix, but it should be in the prefix. - The
AST
was incorrectly camel-cased and should beAst
. - It was confusing next to the expressions plugin's types (
ExpressionFunctionAST
&ExpressionAstFunction
,ExpressionArgAST
&ExpressionAstArgument
).
I think they are named better know:
- Clear
Ast
andExpressionAst
prefixes. - Consistent with each other and with the expressions plugin types.
WDYT?
💛 Build succeeded, but was flaky
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
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.
Presentation Changes look good to me 👍
Summary
This PR resolves #42829, and namely adds the following:
// something
);/* something */
);The comments do not end up in the AST so that there are no changes in the interpreter.
Checklist
For maintainers