From ac728e4e7df66a7aed94c0e7e268a0f91ecad6a9 Mon Sep 17 00:00:00 2001 From: Joe Fleming Date: Fri, 6 Jul 2018 13:20:31 -0700 Subject: [PATCH] Feat: format output from toExpression (#663) * chore: minor code refactor * feat: change the function separator use newlines before the pipes, instead of just keeping everything on 1 line * feat: naive argument formatting put arguments on new line when existing line is over 80 characters * test: fix the toExpression output check * chore: update element expressions match the new formatting rules so changing values from the sidebar is less jarring * fix: handle nested formatting differently * fix: don't wrap subexpression args * chore: typo fix * chore: attempt to make arg value formatting clearer break up the nested ternary, add comments --- common/lib/__tests__/ast.toExpression.js | 10 ++-- common/lib/ast.js | 62 +++++++++++++++------- public/elements/debug/debug.js | 3 +- public/elements/dropdown_filter/index.js | 3 +- public/elements/image/image.js | 3 +- public/elements/markdown/markdown.js | 4 +- public/elements/pie/pie.js | 6 ++- public/elements/plot/plot.js | 7 ++- public/elements/repeatImage/index.js | 6 ++- public/elements/revealImage/index.js | 7 ++- public/elements/table/table.js | 5 +- public/elements/time_filter/time_filter.js | 3 +- 12 files changed, 83 insertions(+), 36 deletions(-) diff --git a/common/lib/__tests__/ast.toExpression.js b/common/lib/__tests__/ast.toExpression.js index f7228ed65b059..eae83e281826d 100644 --- a/common/lib/__tests__/ast.toExpression.js +++ b/common/lib/__tests__/ast.toExpression.js @@ -457,10 +457,10 @@ describe('ast toExpression', () => { const expression = toExpression(astObj); const expected = [ - 'csv input="year,make,model,price', + 'csv \n input="year,make,model,price', '2016,honda,cr-v,23845', '2016,honda,fit,15890,', - '2016,honda,civic,18640" | line x={distinct f="year"} y={sum f="price"} colors={distinct f="model"}', + '2016,honda,civic,18640"\n| line x={distinct f="year"} y={sum f="price"} colors={distinct f="model"}', ]; expect(expression).to.equal(expected.join('\n')); }); @@ -551,11 +551,11 @@ describe('ast toExpression', () => { const expression = toExpression(astObj); const expected = [ - 'csv input="year,make,model,price', + 'csv \n input="year,make,model,price', '2016,honda,cr-v,23845', '2016,honda,fit,15890,', - '2016,honda,civic,18640" | pointseries x={distinct f="year"} y={sum f="price"} ' + - 'colors={distinct f="model"} | line pallette={getColorPallette name="elastic"}', + '2016,honda,civic,18640"\n| pointseries x={distinct f="year"} y={sum f="price"} ' + + 'colors={distinct f="model"}\n| line pallette={getColorPallette name="elastic"}', ]; expect(expression).to.equal(expected.join('\n')); }); diff --git a/common/lib/ast.js b/common/lib/ast.js index b14778021d4f6..624d8da709236 100644 --- a/common/lib/ast.js +++ b/common/lib/ast.js @@ -1,39 +1,60 @@ import { getType } from '../lib/get_type'; import { parse } from './grammar'; -function getArgumentString(arg, argKey) { +function getArgumentString(arg, argKey, level = 0) { const type = getType(arg); - function maybeArgKey(argString) { + function maybeArgKey(argKey, argString) { return argKey == null || argKey === '_' ? argString : `${argKey}=${argString}`; } - function escapeSpecialCharacters(string) { - return string.replace(/[\\"]/g, '\\$&'); // $& means the whole matched string + if (type === 'string') { + // correctly (re)escape double quotes + const escapedArg = arg.replace(/[\\"]/g, '\\$&'); // $& means the whole matched string + return maybeArgKey(argKey, `"${escapedArg}"`); } - if (type === 'string') return maybeArgKey(`"${escapeSpecialCharacters(arg)}"`); - if (type === 'boolean' || type === 'null' || type === 'number') return maybeArgKey(`${arg}`); - if (type === 'expression') return maybeArgKey(`{${getExpression(arg.chain)}}`); + if (type === 'boolean' || type === 'null' || type === 'number') { + // use values directly + return maybeArgKey(argKey, `${arg}`); + } + + if (type === 'expression') { + // build subexpressions + return maybeArgKey(argKey, `{${getExpression(arg.chain, level + 1)}}`); + } + // unknown type, throw with type value throw new Error(`Invalid argument type in AST: ${type}`); } -function getExpressionArgs(block) { +function getExpressionArgs(block, level = 0) { const args = block.arguments; + const hasValidArgs = typeof args === 'object' && args != null && !Array.isArray(args); - if (!hasValidateArgs(args)) throw new Error('Arguments can only be an object'); + if (!hasValidArgs) throw new Error('Arguments can only be an object'); const argKeys = Object.keys(args); - return argKeys.map(argKey => { - const multiArgs = args[argKey]; + const MAX_LINE_LENGTH = 80; // length before wrapping arguments + return argKeys.map(argKey => + args[argKey].reduce((acc, arg) => { + const argString = getArgumentString(arg, argKey, level); + const lineLength = acc.split('\n').pop().length; + + // if arg values are too long, move it to the next line + if (level === 0 && lineLength + argString.length > MAX_LINE_LENGTH) { + return `${acc}\n ${argString}`; + } - return multiArgs.reduce((acc, arg) => acc.concat(getArgumentString(arg, argKey)), []).join(' '); - }); -} + // append arg values to existing arg values + if (lineLength > 0) { + return `${acc} ${argString}`; + } -function hasValidateArgs(args) { - return typeof args === 'object' && args != null && !Array.isArray(args); + // start the accumulator with the first arg value + return argString; + }, '') + ); } function fnWithArgs(fnName, args) { @@ -41,9 +62,12 @@ function fnWithArgs(fnName, args) { return `${fnName} ${args.join(' ')}`; } -function getExpression(chain) { +function getExpression(chain, level = 0) { if (!chain) throw new Error('Expressions must contain a chain'); + // break new functions onto new lines if we're not in a nested/sub-expression + const separator = level > 0 ? ' | ' : '\n| '; + return chain .map(chainObj => { const type = getType(chainObj); @@ -52,12 +76,12 @@ function getExpression(chain) { const fn = chainObj.function; if (!fn || fn.length === 0) throw new Error('Functions must have a function name'); - const expArgs = getExpressionArgs(chainObj); + const expArgs = getExpressionArgs(chainObj, level); return fnWithArgs(fn, expArgs); } }, []) - .join(' | '); + .join(separator); } export function fromExpression(expression, type = 'expression') { diff --git a/public/elements/debug/debug.js b/public/elements/debug/debug.js index 1ec2531eae906..d24dcc9320e14 100644 --- a/public/elements/debug/debug.js +++ b/public/elements/debug/debug.js @@ -5,5 +5,6 @@ export const debug = () => ({ displayName: 'Debug', help: 'Just dumps the configuration of the element', image: header, - expression: 'demodata | render as=debug', + expression: `demodata +| render as=debug`, }); diff --git a/public/elements/dropdown_filter/index.js b/public/elements/dropdown_filter/index.js index c5022923a4cd7..ca7864d1b7431 100644 --- a/public/elements/dropdown_filter/index.js +++ b/public/elements/dropdown_filter/index.js @@ -6,6 +6,7 @@ export const dropdownFilter = () => ({ help: 'A dropdown from which you can select values for an "exactly" filter', image: header, height: 50, - expression: 'demodata | dropdownControl valueColumn=project filterColumn=project', + expression: `demodata +| dropdownControl valueColumn=project filterColumn=project`, filter: '', }); diff --git a/public/elements/image/image.js b/public/elements/image/image.js index 5e6bfd9b5586b..e86d4cc9f2526 100644 --- a/public/elements/image/image.js +++ b/public/elements/image/image.js @@ -5,5 +5,6 @@ export const image = () => ({ displayName: 'Image', help: 'A static image.', image: header, - expression: 'image mode="contain" | render', + expression: `image mode="contain" +| render`, }); diff --git a/public/elements/markdown/markdown.js b/public/elements/markdown/markdown.js index e936b24a35c60..06426581ec4c4 100644 --- a/public/elements/markdown/markdown.js +++ b/public/elements/markdown/markdown.js @@ -5,7 +5,9 @@ export const markdown = () => ({ displayName: 'Markdown', help: 'Markup from Markdown', image: header, - expression: `filters | demodata | markdown "### Welcome to the Markdown Element. + expression: `filters +| demodata +| markdown "### Welcome to the Markdown Element. Good news! You're already connected to some demo data! diff --git a/public/elements/pie/pie.js b/public/elements/pie/pie.js index a53c5bc4a3aea..2f321475d6530 100644 --- a/public/elements/pie/pie.js +++ b/public/elements/pie/pie.js @@ -7,5 +7,9 @@ export const pie = () => ({ height: 300, help: 'An customizable element for making pie charts from your data', image: header, - expression: 'filters | demodata | pointseries color="state" size="max(price)" | pie | render', + expression: `filters +| demodata +| pointseries color="state" size="max(price)" +| pie +| render`, }); diff --git a/public/elements/plot/plot.js b/public/elements/plot/plot.js index adccff9e1e0d8..229f62436f770 100644 --- a/public/elements/plot/plot.js +++ b/public/elements/plot/plot.js @@ -5,6 +5,9 @@ export const plot = () => ({ displayName: 'Coordinate plot', help: 'An customizable XY plot for making line, bar or dot charts from your data', image: header, - expression: - 'filters | demodata | pointseries x="time" y="sum(price)" color="state" | plot defaultStyle={seriesStyle points=5} | render', + expression: `filters +| demodata +| pointseries x="time" y="sum(price)" color="state" +| plot defaultStyle={seriesStyle points=5} +| render`, }); diff --git a/public/elements/repeatImage/index.js b/public/elements/repeatImage/index.js index 7e2e0b99de85e..1ed7b59979cab 100644 --- a/public/elements/repeatImage/index.js +++ b/public/elements/repeatImage/index.js @@ -5,5 +5,9 @@ export const repeatImage = () => ({ displayName: 'Image Repeat', help: 'Repeats an image N times', image: header, - expression: 'demodata | pointseries size="mean(cost)" | getCell | repeatImage | render', + expression: `demodata +| pointseries size="mean(cost)" +| getCell +| repeatImage +| render`, }); diff --git a/public/elements/revealImage/index.js b/public/elements/revealImage/index.js index 67f7b6b01d05a..00ac8031dbb2a 100644 --- a/public/elements/revealImage/index.js +++ b/public/elements/revealImage/index.js @@ -5,6 +5,9 @@ export const revealImage = () => ({ displayName: 'Image Reveal', help: 'Reveals a percentage of an image', image: header, - expression: - 'demodata | pointseries size="sum(min(cost) / max(cost))" | getCell | revealImage origin=bottom | render', + expression: `demodata +| pointseries size="sum(min(cost) / max(cost))" +| getCell +| revealImage origin=bottom +| render`, }); diff --git a/public/elements/table/table.js b/public/elements/table/table.js index 5061e0f6a635d..b17cd41e738de 100644 --- a/public/elements/table/table.js +++ b/public/elements/table/table.js @@ -5,5 +5,8 @@ export const table = () => ({ displayName: 'Data Table', help: 'A scrollable grid for displaying data in a tabular format', image: header, - expression: 'filters | demodata | table | render', + expression: `filters +| demodata +| table +| render`, }); diff --git a/public/elements/time_filter/time_filter.js b/public/elements/time_filter/time_filter.js index 4641fefc2ce59..1d8eb5f06ff4e 100644 --- a/public/elements/time_filter/time_filter.js +++ b/public/elements/time_filter/time_filter.js @@ -6,6 +6,7 @@ export const timeFilter = () => ({ help: 'Set a time window', image: header, height: 50, - expression: 'timefilterControl compact=true column=@timestamp | render as=time_filter', + expression: `timefilterControl compact=true column=@timestamp +| render as=time_filter`, filter: 'timefilter column=@timestamp from=now-24h to=now', });