Skip to content
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

Rewrite pipeline builder #51246

Closed
1 of 3 tasks
streamich opened this issue Nov 21, 2019 · 3 comments
Closed
1 of 3 tasks

Rewrite pipeline builder #51246

streamich opened this issue Nov 21, 2019 · 3 comments
Assignees
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Visualizations Generic visualization features (in case no more specific feature label is available)

Comments

@streamich
Copy link
Contributor

streamich commented Nov 21, 2019

  • Depends on: Expression AST builder #56748
  • Generate AST instead of expression strings when building expressions for visualizations.
  • Update / add more test coverage

Parent issue: #46909

@streamich streamich added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Nov 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers
Copy link
Member

coping @ppisljar notes over from duplicate issue #61829:


currently we build expressions for visualizations in build_pipeline.ts by concatenating strings. It has two problems:

  1. string concatenation is easy to break and hard to test
  2. impossible to extend (for 3rd party visualizations)

we should add toExpression or toAST function to visualization definition which accepts VisParams and returns correct AST.

@lukeelmers lukeelmers added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label May 5, 2020
@lukeelmers lukeelmers self-assigned this May 5, 2020
@lukeelmers
Copy link
Member

The remaining work for this was started awhile back in #65396, but then put on hold while we finished work on the Expressions AST Builder which could be used in the pipeline builder.

@timroes and I discussed this offline -- Now that the app team is taking over the visualizations plugin, they will assess whether this refactoring is still necessary. After more of the visualizations have implemented the toExpressionAst method, there will be very little left in this file, and it might even be removed entirely.

To avoid stepping on the app team's toes, I will go ahead and close this issue as we no longer need to track it in the app arch expressions roadmap (plus it really has more to do with visualizations anyway).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Visualizations Generic visualization features (in case no more specific feature label is available)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants