Timelion typeahead for argument values #14801

nreese · 2017-11-06T17:36:55Z

Adds typeahead support for timelion argument values, #9730

cjcenizal · 2017-11-06T23:23:42Z

I think the bug causing the functional test failures has been fixed in master.

cjcenizal

Wow, nice work man! This is a solid improvement.

I haven't finished going through everything but I had a couple of more fundamental changes I wanted to share with you before I went any further. Love to hear your thoughts.

cjcenizal · 2017-11-07T00:30:00Z

src/core_plugins/timelion/public/directives/timelion_expression_input_helpers.js

+      const message = JSON.parse(e.message);
+      const location = message.location;
+
+      if (message.type === 'incompleteFunction') {


Maybe this should be a switch/case now?

cjcenizal · 2017-11-07T00:41:42Z

..._plugins/timelion/public/directives/timelion_expression_suggestions/arg_value_suggestions.js

+      }
+
+      return help.suggestions;
+    }


I was having a hard time following the logic around the conditions within this function. I think we could make this a little easier to follow if we split this up into three functions, and made the consuming code responsible for handling the condition. This will probably be easier to test, too.

return { hasDynamicSuggestionsForArgument: (functionName, argName) => { return (customHandlers[functionName] && customHandlers[functionName][argName]); }, getDynamicSuggestionsForArgument: async (functionName, argName, functionArgs, partialInput = '') => { return await customHandlers[functionName][argName](partialInput, functionArgs); }, getStaticSuggestionsForInput: (partialInput = '', staticSuggestions = []) => { if (partialInput) { return staticSuggestions.filter(suggestion => { return suggestion.name.includes(partialInput); }); } return staticSuggestions; }, };

cjcenizal · 2017-11-07T01:21:01Z

src/core_plugins/timelion/public/directives/timelion_expression_input_helpers.js


  const activeArg = activeFunc.arguments.find((argument) => {
    return inLocation(cursorPosition, argument.location);
  });
  // return argument_value suggestions when cursor is inside agrument value
  if (activeArg && activeArg.type === 'namedArg' && inLocation(cursorPosition, activeArg.value.location)) {
-    // TODO - provide argument value suggestions once function list contains required data
-    return { list: [], location: activeArg.value.location, type: SUGGESTION_TYPE.ARGUMENT_VALUE };
+    const valueSuggestions = await getArgValueSuggestions(


This would become:

const { function: functionName, arguments: functionArgs, } = activeFunc; const { name: argName, value: { text: partialInput }, } = activeArg; let valueSuggestions; if (hasDynamicSuggestionsForArgument(functionName, argName)) { valueSuggestions = await getDynamicSuggestionsForArgument(functionName, argName, functionArgs, partialInput); } else { const { suggestions: staticSuggestions, } = functionHelp.args.find((arg) => { return arg.name === activeArg.name; }); valueSuggestions = getStaticSuggestionsForInput(partialInput, staticSuggestions); }

This comment pertains to my last comment at the bottom.

cjcenizal · 2017-11-07T01:22:04Z

src/core_plugins/timelion/public/directives/timelion_expression_input_helpers.js

-      } catch (e) {
-        // The expression isn't correctly formatted, so JSON.parse threw an error.
-        return reject();
+        const valueSuggestions = await getArgValueSuggestions(message.name, null, argHelp, message.currentFunction, message.currentArgs);


And this becomes:

const { name: argName, currentFunction: functionName, currentArgs: functionArgs, } = message; let valueSuggestions = []; if (hasDynamicSuggestionsForArgument(functionName, argName)) { valueSuggestions = await getDynamicSuggestionsForArgument(functionName, argName, functionArgs); } else { const functionHelp = functionList.find(func => func.name === functionName); if (functionHelp) { valueSuggestions = functionHelp.args.find(arg => arg.name === argName); } }

This comment pertains to my last comment at the bottom.

cjcenizal · 2017-11-07T01:44:47Z

src/core_plugins/timelion/public/directives/timelion_expression_input.js

+            const newCaretOffset = min + argumentName.length;
+            setCaretOffset(newCaretOffset);
+            break;
+          }


There's a lot of repetition here. I think this becomes a little more maintainable if we extract the repetitive parts. I haven't tested this btw.

const { min, max } = suggestibleFunctionLocation; let insertedValue; let insertPositionMinOffset = 0; switch (scope.suggestions.type) { case SUGGESTION_TYPE.FUNCTIONS: { // Position the caret inside of the function parentheses. insertedValue = `${scope.suggestions.list[suggestionIndex].name}()`; // Update the expression with the function. // min advanced one to not replace function '.' insertPositionMinOffset = 1; break; } case SUGGESTION_TYPE.ARGUMENTS: { // Position the caret after the '=' insertedValue = `${scope.suggestions.list[suggestionIndex].name}=`; break; } case SUGGESTION_TYPE.ARGUMENT_VALUE: { // Position the caret after the '=' insertedValue = `${scope.suggestions.list[suggestionIndex].name}`; break; } } const updatedExpression = insertAtLocation(insertedValue, scope.sheet, min + insertPositionMinOffset, max); scope.sheet = updatedExpression; const newCaretOffset = min + insertedValue.length; setCaretOffset(newCaretOffset);

I had meant to go back and out the duplicated code to clean this switch statement but just forgot about it. Thanks for cleaning it up. Your code block worked great

cjcenizal

LGTM! Had a couple nitpicky suggestions but feel free to disregard if you don't like them.

cjcenizal · 2017-11-10T23:09:15Z

src/core_plugins/timelion/public/directives/timelion_expression_input_helpers.js

-      activeArg.name,
-      activeArg.value.text,
-      functionHelp.args.find((arg) => {
+    const {


It might be nice to destructure the methods in argValueSuggestions, too.

const { hasDynamicSuggestionsForArgument, getDynamicSuggestionsForArgument, getStaticSuggestionsForInput, } = argValueSuggestions;

This could make the code on lines 112 through 120 a bit terser.

cjcenizal · 2017-11-10T23:09:34Z

src/core_plugins/timelion/public/directives/timelion_expression_input_helpers.js

+        return { list, location: message.location, type: SUGGESTION_TYPE.FUNCTIONS };
+      }
+      case 'incompleteArgument': {
+        const {


Same suggestion about destructuring argValueSuggestions applies here.

cjcenizal

Found a couple more minor things.

cjcenizal · 2017-11-10T23:14:49Z

src/core_plugins/timelion/server/series_functions/fit.js

@@ -13,7 +13,10 @@ export default new Chainable('fit', {
    {
      name: 'mode',
      types: ['string'],
-      help: 'The algorithm to use for fitting the series to the target. One of: ' + _.keys(fitFunctions).join(', ')
+      help: 'The algorithm to use for fitting the series to the target. One of: ' + _.keys(fitFunctions).join(', '),


You can use a template literal here.

cjcenizal · 2017-11-10T23:15:03Z

src/core_plugins/timelion/server/series_functions/trend/index.js

@@ -16,7 +16,10 @@ export default new Chainable('trend', {
    {
      name: 'mode',
      types: ['string'],
-      help: 'The algorithm to use for generating the trend line. One of: ' + _.keys(validRegressions).join(', ')
+      help: 'The algorithm to use for generating the trend line. One of: ' + _.keys(validRegressions).join(', '),


cjcenizal · 2017-11-10T23:15:08Z

src/core_plugins/timelion/server/series_functions/yaxis.js

@@ -50,7 +50,10 @@ export default new Chainable('yaxis', {
    {
      name: 'units',
      types: ['string', 'null'],
-      help: 'The function to use for formatting y-axis labels. One of: ' + _.values(tickFormatters).join(', ')
+      help: 'The function to use for formatting y-axis labels. One of: ' + _.values(tickFormatters).join(', '),


timroes

LGTM

…ration

…ngle getSuggetions method

* timelion argument value suggestions for legend function * update functions with argument value suggestions * use async/await instead of promise resolve/reject for suggestion generation * lookup value suggestions for es index and timefield arguments * custom handlers for es metric and split arguments * update suggestions unit test * remove duplicate code from insertSuggestion switch * refactor arg_value_suggestions to provide three methods instead of single getSuggetions method * template literal * update es index argument desc to include note about type ahead support

nreese added Feature:Timelion Timelion app and visualization release_note:enhancement v6.1.0 v7.0.0 labels Nov 6, 2017

nreese requested review from timroes and cjcenizal November 6, 2017 17:36

nreese force-pushed the arg_value_suggestions branch from a77295a to cf746f6 Compare November 7, 2017 01:13

cjcenizal suggested changes Nov 7, 2017

View reviewed changes

cjcenizal mentioned this pull request Nov 9, 2017

Address Timelion suggestion edge cases #14888

Closed

cjcenizal approved these changes Nov 10, 2017

View reviewed changes

nreese force-pushed the arg_value_suggestions branch from 772eea2 to c99b08c Compare November 13, 2017 14:59

timroes approved these changes Nov 14, 2017

View reviewed changes

nreese added 10 commits November 14, 2017 10:10

timelion argument value suggestions for legend function

96230be

update functions with argument value suggestions

ad8af48

use async/await instead of promise resolve/reject for suggestion gene…

8e9711d

…ration

lookup value suggestions for es index and timefield arguments

b31313a

custom handlers for es metric and split arguments

bb71325

update suggestions unit test

9b521b3

remove duplicate code from insertSuggestion switch

e464355

refactor arg_value_suggestions to provide three methods instead of si…

2deb1a2

…ngle getSuggetions method

template literal

99f7f60

update es index argument desc to include note about type ahead support

d833bdc

nreese force-pushed the arg_value_suggestions branch from 7f92145 to d833bdc Compare November 14, 2017 17:10

nreese merged commit 94b2b32 into elastic:master Nov 14, 2017

nreese mentioned this pull request Nov 14, 2017

[Backport] Timelion typeahead for argument values (#14801) #14945

Closed

nreese mentioned this pull request Nov 15, 2017

[Backport] Timelion typeahead for argument values (#14801) #14958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timelion typeahead for argument values #14801

Timelion typeahead for argument values #14801

nreese commented Nov 6, 2017 •

edited

Loading

cjcenizal commented Nov 6, 2017

cjcenizal left a comment

cjcenizal Nov 7, 2017

cjcenizal Nov 7, 2017

cjcenizal Nov 7, 2017

cjcenizal Nov 7, 2017 •

edited

Loading

cjcenizal Nov 7, 2017

cjcenizal Nov 7, 2017 •

edited

Loading

cjcenizal Nov 7, 2017

nreese Nov 7, 2017

cjcenizal left a comment

cjcenizal Nov 10, 2017

cjcenizal Nov 10, 2017

cjcenizal left a comment

cjcenizal Nov 10, 2017

cjcenizal Nov 10, 2017

cjcenizal Nov 10, 2017

timroes left a comment

Timelion typeahead for argument values #14801

Timelion typeahead for argument values #14801

Conversation

nreese commented Nov 6, 2017 • edited Loading

cjcenizal commented Nov 6, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

nreese commented Nov 6, 2017 •

edited

Loading

cjcenizal Nov 7, 2017 •

edited

Loading

cjcenizal Nov 7, 2017 •

edited

Loading