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

variable support for interpreter #54788

Merged
merged 3 commits into from
Jan 15, 2020

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jan 14, 2020

Summary

As already defined in the types, interpreter handlers can contain the list of variables. This PR adds the implementation needed for that to work:

  • variables can be passed in as a configuration to expression executor or expression loader
  • var function is added to the function registry, which has a single argument name and it would read that variable from the handlers.variables and return it.
  • var_set (looking for better name suggestions) accepts a name and optionally a value and would update the variable with this value. If value is not provided context will be used as variable value. var_set function returns the same context it received (so it does not affect the following functions, unless they are using a variable that was updated)

when running the expression the app can decide if it wants local variables (dashboard would pass separate instance of variable object to each embeddable) or global variables shared among embeddables (same instance of variables object would be passed to all embeddables)

Closes #46908

Example

using variables provided by the app executing the expression:
esaggs indexPattern={var indexPattern} aggs={....} | ...

reusing context within another expression
esaggs .... | var_set storedContext | pie ....
var storedContext | table ...

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

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

This was referenced Jan 14, 2020
@ppisljar ppisljar force-pushed the expressions/variables branch from 2164002 to 97677aa Compare January 15, 2020 13:25
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, given the value falsy check, see below.

Also, would be nice to add unit tests for both functions.

},
async fn(context, args, handlers) {
const variables: Record<string, any> = handlers.variables;
if (!variables[args.name]) {
Copy link
Contributor

@streamich streamich Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case value is falsy, like 0:

Suggested change
if (!variables[args.name]) {
if (variables[args.name] === undefined) {

or

Suggested change
if (!variables[args.name]) {
if (!variables.hasOwnProperty(args.name)) {

or maybe we could use a Map

Suggested change
if (!variables[args.name]) {
if (!variables.has(args.name)) {

async fn(context, args, handlers) {
const variables: Record<string, any> = handlers.variables;
if (!variables[args.name]) {
throw new Error(`Variable "${args.name}" does not exist`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we throw here? Or maybe we could return undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, thanks!

@streamich
Copy link
Contributor

streamich commented Jan 15, 2020

(looking for better name suggestions)

For setting:

  • def
  • set
  • var
  • val
  • let
  • export
  • define

For using:

  • get
  • use
  • var
  • val
  • $

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ppisljar ppisljar merged commit 5d4cb47 into elastic:master Jan 15, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Jan 15, 2020
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
@bhavyarm
Copy link
Contributor

@ppisljar how do we test this pr? Thanks!

@ppisljar
Copy link
Member Author

this is currently not exposed in any way to the user, so its can't really be tested and should not have any implications for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expressions plugin variables
5 participants