From 3ef547fbb73bc10e83a4d7259092d0d9603ffccc Mon Sep 17 00:00:00 2001 From: Alexis Lacroix Date: Mon, 6 Jan 2025 09:35:08 +0100 Subject: [PATCH 1/2] [IMP] formulas: changes args function targetting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want to introduce new formulas whose definitions may deviate from traditional patterns. For instance, we might develop a SWITCH formula where an optional argument appears after repeatable arguments. We also foresee future formulas with mandatory arguments following optional arguments. However, our current logic cannot properly map values to their corresponding arguments. Moreover, we are not entirely sure what other “exotic” configurations might exist and how they would behave. This commit introduces a completely new way of mapping values to arguments, making it possible to handle any configuration—even the most unconventional. Henceforth, we do not have to worry about the order in which - mandatory, optional, and repeatable - arguments appear; they can be mixed in any manner the developer chooses. The only constraint we enforce is that repeatable arguments must be declared in groups, and, if repeatable arguments are present, their total number must exceed that of the optional arguments. These changes also impact the formula composer: By supporting formulas with mandatory or optional arguments placed after repeatable ones, any given argument value the user enters might correspond to either a repeatable argument or a mandatory/optional argument. The argument to focus on in the composer thus depends on how many arguments are ultimately passed to the function, requiring us to potentially focus multiple arguments at once. Lastly, we addressed a conflict with an existing “exotic” formula, SORTN (from Google Sheets), which originally included two optional arguments alongside two repeatable arguments. To make these new rules work, one of the optional arguments was converted into a mandatory argument. Task: 3829128 Co-authored-by: anhe-odoo --- src/components/composer/composer/composer.ts | 73 +++- src/components/composer/composer/composer.xml | 2 +- .../formula_assistant/formula_assistant.ts | 4 +- .../formula_assistant/formula_assistant.xml | 7 +- src/formulas/compiler.ts | 39 +- src/formulas/composer_tokenizer.ts | 6 +- src/functions/arguments.ts | 207 +++++++--- src/functions/index.ts | 11 +- src/functions/module_filter.ts | 16 +- src/functions/module_lookup.ts | 8 +- src/types/functions.ts | 2 +- .../formula_assistant_component.test.ts | 275 ++++++++++++- tests/evaluation/compiler.test.ts | 6 +- tests/functions/arguments.test.ts | 366 +++++++++++++++--- tests/functions/module_filter.test.ts | 54 ++- .../spreadsheet_pivot.test.ts | 2 +- 16 files changed, 890 insertions(+), 188 deletions(-) diff --git a/src/components/composer/composer/composer.ts b/src/components/composer/composer/composer.ts index 03a0103b7e..1aa60b5bfc 100644 --- a/src/components/composer/composer/composer.ts +++ b/src/components/composer/composer/composer.ts @@ -4,6 +4,7 @@ import { functionRegistry } from "../../../functions/index"; import { clip, setColorAlpha } from "../../../helpers/index"; import { EnrichedToken } from "../../../formulas/composer_tokenizer"; +import { argTargeting } from "../../../functions/arguments"; import { Store, useLocalStore, useStore } from "../../../store_engine"; import { DOMFocusableElementStore } from "../../../stores/DOM_focus_store"; import { @@ -137,7 +138,7 @@ interface FunctionDescriptionState { showDescription: boolean; functionName: string; functionDescription: FunctionDescription; - argToFocus: number; + argsToFocus: number[]; } export class Composer extends Component { @@ -179,7 +180,7 @@ export class Composer extends Component showDescription: false, functionName: "", functionDescription: {} as FunctionDescription, - argToFocus: 0, + argsToFocus: [], }); assistant = useState({ forcedClosed: false, @@ -712,7 +713,8 @@ export class Composer extends Component * the autocomplete engine otherwise we initialize the formula assistant. */ private processTokenAtCursor(): void { - let content = this.props.composerStore.currentContent; + const composerStore = this.props.composerStore; + let content = composerStore.currentContent; if (this.autoCompleteState.provider) { this.autoCompleteState.hide(); } @@ -735,15 +737,78 @@ export class Composer extends Component // initialize Formula Assistant const description = functions[parentFunction]; const argPosition = tokenContext.argPosition; + const nbrArgSupplied = tokenContext.args.length; this.functionDescriptionState.functionName = parentFunction; this.functionDescriptionState.functionDescription = description; - this.functionDescriptionState.argToFocus = description.getArgToFocus(argPosition + 1) - 1; + + const isParenthesisClosed = !!this.props.composerStore.currentTokens.find( + (t) => t.type === "RIGHT_PAREN" && t.parenthesesCode === token.parenthesesCode + ); + + this.functionDescriptionState.argsToFocus = this.getArgsToFocus( + isParenthesisClosed, + description, + nbrArgSupplied, + argPosition + ); + this.functionDescriptionState.showDescription = true; } } } + /** + * Compute the argument to focus depending on the current value position. + * + * This function is useful to indicate in the functionDescriptionState which argument should be focused. + * Normally, 'argTargeting' is used to compute the argument to focus, but in the composer, + * we don't yet know how many arguments the user will supply. + * + * This function will compute all the possible arguments to focus for different numbers of arguments supplied. + */ + private getArgsToFocus( + isParenthesisClosed: boolean, + description: FunctionDescription, + nbrArgSupplied: number, + argPosition: number + ): number[] { + // When the parenthesis is closed, we consider the user is done with the function, + // so we know exactly the number of arguments supplied. + + if (isParenthesisClosed) { + const focusedArg = argTargeting( + description, + Math.max(Math.min(description.maxArgPossible, nbrArgSupplied), description.minArgRequired) + )(argPosition); + return focusedArg !== undefined ? [focusedArg] : []; + } + + // Otherwise, the user is still typing the formula, so we don't know yet how many arguments the user will supply. + // Consequently, we need to compute not only the argument to focus for the current number of arguments supplied + // but also for all the possible numbers of arguments supplied. + + const minArgsNumberPossibility = Math.max(nbrArgSupplied, description.minArgRequired); + const maxArgsNumberPossibility = description.nbrArgRepeating + ? description.minArgRequired + + Math.ceil( + (minArgsNumberPossibility - description.minArgRequired) / description.nbrArgRepeating + ) * + description.nbrArgRepeating + + description.nbrArgOptional + : description.maxArgPossible; + + const argsToFocus: number[] = []; + for (let i = minArgsNumberPossibility; i <= maxArgsNumberPossibility; i++) { + const focusedArg = argTargeting(description, i)(argPosition); + if (focusedArg !== undefined) { + argsToFocus.push(focusedArg); + } + } + + return [...new Set(argsToFocus)]; + } + private autoComplete(value: string) { if (!value || this.assistant.forcedClosed) { return; diff --git a/src/components/composer/composer/composer.xml b/src/components/composer/composer/composer.xml index 61e9f14854..ae56769eb0 100644 --- a/src/components/composer/composer/composer.xml +++ b/src/components/composer/composer/composer.xml @@ -60,7 +60,7 @@ t-if="functionDescriptionState.showDescription" functionName="functionDescriptionState.functionName" functionDescription="functionDescriptionState.functionDescription" - argToFocus="functionDescriptionState.argToFocus" + argsToFocus="functionDescriptionState.argsToFocus" />
{ @@ -50,7 +50,7 @@ export class FunctionDescriptionProvider extends Component, }; getContext(): Props { diff --git a/src/components/composer/formula_assistant/formula_assistant.xml b/src/components/composer/formula_assistant/formula_assistant.xml index ae1fb8f8ba..371f589b4b 100644 --- a/src/components/composer/formula_assistant/formula_assistant.xml +++ b/src/components/composer/formula_assistant/formula_assistant.xml @@ -9,7 +9,8 @@ ( - + [ @@ -37,8 +38,8 @@
diff --git a/src/formulas/compiler.ts b/src/formulas/compiler.ts index 7142a6ca9d..702aafa4de 100644 --- a/src/formulas/compiler.ts +++ b/src/formulas/compiler.ts @@ -1,4 +1,5 @@ import { Token } from "."; +import { argTargeting } from "../functions/arguments"; import { functionRegistry } from "../functions/index"; import { parseNumber, removeStringQuotes, unquote } from "../helpers"; import { _t } from "../translation"; @@ -125,9 +126,10 @@ function compileTokensOrThrow(tokens: Token[]): CompiledFormula { const compiledArgs: FunctionCode[] = []; + const argToFocus = argTargeting(functionDefinition, args.length); + for (let i = 0; i < args.length; i++) { - const argToFocus = functionDefinition.getArgToFocus(i + 1) - 1; - const argDefinition = functionDefinition.args[argToFocus]; + const argDefinition = functionDefinition.args[argToFocus(i) ?? -1]; const currentArg = args[i]; const argTypes = argDefinition.type || []; @@ -316,43 +318,48 @@ function formulaArguments(tokens: Token[]) { * Check if arguments are supplied in the correct quantities */ function assertEnoughArgs(ast: ASTFuncall) { - const nbrArg = ast.args.length; + const nbrArgSupplied = ast.args.length; const functionName = ast.value.toUpperCase(); const functionDefinition = functions[functionName]; + const nbrArgRepeating = functionDefinition.nbrArgRepeating; + const minArgRequired = functionDefinition.minArgRequired; - if (nbrArg < functionDefinition.minArgRequired) { + if (nbrArgSupplied < minArgRequired) { throw new BadExpressionError( _t( "Invalid number of arguments for the %s function. Expected %s minimum, but got %s instead.", functionName, - functionDefinition.minArgRequired.toString(), - nbrArg.toString() + minArgRequired.toString(), + nbrArgSupplied.toString() ) ); } - if (nbrArg > functionDefinition.maxArgPossible) { + if (nbrArgSupplied > functionDefinition.maxArgPossible) { throw new BadExpressionError( _t( "Invalid number of arguments for the %s function. Expected %s maximum, but got %s instead.", functionName, functionDefinition.maxArgPossible.toString(), - nbrArg.toString() + nbrArgSupplied.toString() ) ); } - const repeatableArgs = functionDefinition.nbrArgRepeating; - if (repeatableArgs > 1) { - const unrepeatableArgs = functionDefinition.args.length - repeatableArgs; - const repeatingArgs = nbrArg - unrepeatableArgs; - if (repeatingArgs % repeatableArgs !== 0) { + if (nbrArgRepeating > 1) { + const nbrValueRepeating = + nbrArgRepeating * Math.floor((nbrArgSupplied - minArgRequired) / nbrArgRepeating); + const nbrValueRemaining = + nbrArgSupplied - minArgRequired - nbrValueRepeating - functionDefinition.nbrArgOptional; + + if (nbrValueRemaining > 0) { throw new BadExpressionError( _t( - "Invalid number of arguments for the %s function. Expected all arguments after position %s to be supplied by groups of %s arguments", + "Invalid number of arguments for the %s function. Repeatable arguments are expected to be supplied by groups of %s argument(s) with maximum %s optional argument(s), but got %s argument(s) too many.", functionName, - unrepeatableArgs.toString(), - repeatableArgs.toString() + nbrArgRepeating.toString(), + functionDefinition.nbrArgOptional.toString(), + nbrValueRemaining.toString() ) ); } diff --git a/src/formulas/composer_tokenizer.ts b/src/formulas/composer_tokenizer.ts index 7d5e6fe92c..92dd089d44 100644 --- a/src/formulas/composer_tokenizer.ts +++ b/src/formulas/composer_tokenizer.ts @@ -163,11 +163,11 @@ function mapParentFunction(tokens: EnrichedToken[]): EnrichedToken[] { pushTokenToFunctionContext(token); break; case "ARG_SEPARATOR": - pushTokenToFunctionContext(token); if (stack.length) { // increment position on current function stack[stack.length - 1].argPosition++; } + pushTokenToFunctionContext(token); break; default: pushTokenToFunctionContext(token); @@ -208,8 +208,8 @@ function addArgsAST(tokens: EnrichedToken[]): EnrichedToken[] { } for (const argTokens of argsTokens) { let tokens = argTokens; - if (tokens.at(-1)?.type === "ARG_SEPARATOR") { - tokens = tokens.slice(0, -1); + if (tokens.at(0)?.type === "ARG_SEPARATOR") { + tokens = tokens.slice(1); } try { args.push(parseTokens(tokens)); diff --git a/src/functions/arguments.ts b/src/functions/arguments.ts index b88b15b43d..d1c318a553 100644 --- a/src/functions/arguments.ts +++ b/src/functions/arguments.ts @@ -88,6 +88,7 @@ export function addMetaInfoFromArg(addDescr: AddFunctionDescription): FunctionDe let countArg = 0; let minArg = 0; let repeatingArg = 0; + let optionalArg = 0; for (let arg of addDescr.args) { countArg++; if (!arg.optional && !arg.repeating && !arg.default) { @@ -96,56 +97,145 @@ export function addMetaInfoFromArg(addDescr: AddFunctionDescription): FunctionDe if (arg.repeating) { repeatingArg++; } + if (arg.optional || arg.default) { + optionalArg++; + } } const descr = addDescr as FunctionDescription; descr.minArgRequired = minArg; descr.maxArgPossible = repeatingArg ? Infinity : countArg; descr.nbrArgRepeating = repeatingArg; - descr.getArgToFocus = argTargeting(countArg, repeatingArg); + descr.nbrArgOptional = optionalArg; descr.hidden = addDescr.hidden || false; return descr; } /** - * Returns a function allowing finding which argument corresponds a position - * in a function. This is particularly useful for functions with repeatable - * arguments. + * Returns a function that maps the position of a value in a function to its corresponding argument index. + * This is particularly useful for functions with repeatable and/or optional arguments. + * + * In most cases, the task is straightforward: + * + * In the formula "=SUM(11, 55, 66)" which is defined like this "SUM(value1, [value2, ...])": + * - 11 corresponds to the value1 argument => position will be 0 + * - 55 and 66 correspond to the [value2, ...] argument => position will be 1 + * + * In other cases, optional arguments could be defined after repeatable arguments, + * or even optional and required arguments could be mixed in unconventional ways. + * Therefore, it becomes more complex. It is the reason why the corresponding argument index depends + * not only on the position of the value in the function but also on the total number of values passed to the function. + * + * The next function has been designed to handle all possible configurations, + * it has the advantage to not functionally restrict the way the arguments are defined. + * The only restriction is if repeatable arguments are present in the function definition: + * - they must be defined consecutively + * - they must be in a quantity greater than the optional arguments. + * + * The markdown tables below illustrate how values are mapped to positions based on the number of values supplied. + * Each table represents a different function configuration, with columns representing the number of values supplied as arguments + * and rows representing the correspondence with the argument index. + * + * The tables are built based on the following conventions: + * - `m`: Mandatory argument + * - `o`: Optional argument + * - `r`: Repeating argument + * + * + * Configuration 1: (m, o) like the CEILING function + * + * | | 1 | 2 | + * |---|---|---| + * | m | 0 | 0 | + * | o | | 1 | + * + * + * Configuration 2: (m, m, m, r, r) like the SUMIFS function + * + * | | 3 | 5 | 7 | 3 + 2n | + * |---|---|---|------|------------| + * | m | 0 | 0 | 0 | 0 | + * | m | 1 | 1 | 1 | 1 | + * | m | 2 | 2 | 2 | 2 | + * | r | | 3 | 3, 5 | 3 + 2n | + * | r | | 4 | 4, 6 | 3 + 2n + 1 | + * + * + * Configuration 3: (m, m, m, r, r, o) like the SWITCH function + * + * | | 3 | 4 | 5 | 6 | 7 | 8 | 3 + 2n | 3 + 2n + 1 | + * |---|---|---|---|---|------|------|------------|----------------| + * | m | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | + * | m | 1 | 1 | 1 | 1 | 1 | 1 | 1 | 1 | + * | m | 2 | 2 | 2 | 2 | 2 | 2 | 2 | 2 | + * | r | | | 3 | 3 | 3, 5 | 3, 5 | 3 + 2n | 3 + 2n | + * | r | | | 4 | 4 | 4, 6 | 4, 6 | 3 + 2n + 1 | 3 + 2n + 1 | + * | o | | 3 | | 5 | | 7 | | 3 + 2N + 2 | + * + * + * Configuration 4: (m, o, m, o, r, r, r, m) a complex case to understand subtleties + * + * | | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | ... | + * |---|---|---|---|---|---|---|------|------|------|-----| + * | m | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | ... | + * | o | | 1 | 1 | | 1 | 1 | | 1 | 1 | ... | + * | m | 1 | 2 | 2 | 1 | 2 | 2 | 1 | 2 | 2 | ... | + * | o | | | 3 | | | 3 | | | 3 | ... | + * | r | | | | 2 | 3 | 4 | 2, 5 | 3, 6 | 4, 7 | ... | + * | r | | | | 3 | 4 | 5 | 3, 6 | 4, 7 | 5, 8 | ... | + * | r | | | | 4 | 5 | 6 | 4, 7 | 5, 8 | 6, 9 | ... | + * | m | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | ... | * - * Indeed the function makes it possible to etablish corespondance between - * arguments when the number of arguments supplied is greater than the number of - * arguments defined by the function. - * - * Ex: - * - * in the formula "=SUM(11, 55, 66)" which is defined like this "SUM(value1, [value2, ...])" - * - 11 corresponds to the value1 argument => position will be 1 - * - 55 corresponds to the [value2, ...] argument => position will be 2 - * - 66 corresponds to the [value2, ...] argument => position will be 2 - * - * in the formula "=AVERAGE.WEIGHTED(1, 2, 3, 4, 5, 6)" which is defined like this - * "AVERAGE.WEIGHTED(values, weights, [additional_values, ...], [additional_weights, ...])" - * - 1 corresponds to the values argument => position will be 1 - * - 2 corresponds to the weights argument => position will be 2 - * - 3 corresponds to the [additional_values, ...] argument => position will be 3 - * - 4 corresponds to the [additional_weights, ...] argument => position will be 4 - * - 5 corresponds to the [additional_values, ...] argument => position will be 3 - * - 6 corresponds to the [additional_weights, ...] argument => position will be 4 */ -function argTargeting(countArg, repeatingArg): (argPosition: number) => number { - if (!repeatingArg) { - return (argPosition) => argPosition; - } - if (repeatingArg === 1) { - return (argPosition) => Math.min(argPosition, countArg); - } - const argBeforeRepeat = countArg - repeatingArg; - return (argPosition) => { - if (argPosition <= argBeforeRepeat) { - return argPosition; +export function argTargeting( + functionDescription: FunctionDescription, + nbrArgSupplied: number +): (argPosition: number) => number | undefined { + const valueIndexToArgPosition = new Map(); + const groupsOfRepeatingValues = functionDescription.nbrArgRepeating + ? Math.floor( + (nbrArgSupplied - functionDescription.minArgRequired) / functionDescription.nbrArgRepeating + ) + : 0; + const nbrValueRepeating = functionDescription.nbrArgRepeating * groupsOfRepeatingValues; + const nbrValueOptional = nbrArgSupplied - functionDescription.minArgRequired - nbrValueRepeating; + + let countValueSupplied = 0; + let countValueOptional = 0; + + for (let i = 0; i < functionDescription.args.length; i++) { + const arg = functionDescription.args[i]; + + if (arg.optional || arg.default) { + if (countValueOptional < nbrValueOptional) { + valueIndexToArgPosition.set(countValueSupplied, i); + countValueSupplied++; + } + countValueOptional++; + continue; } - const argAfterRepeat = (argPosition - argBeforeRepeat) % repeatingArg || repeatingArg; - return argBeforeRepeat + argAfterRepeat; + + if (arg.repeating) { + // As we know all repeating arguments are consecutive, + // --> we will treat all repeating arguments in one go + // --> the index i will be incremented by the number of repeating values at the end of the loop + for (let j = 0; j < groupsOfRepeatingValues; j++) { + for (let k = 0; k < functionDescription.nbrArgRepeating; k++) { + valueIndexToArgPosition.set(countValueSupplied, i + k); + countValueSupplied++; + } + } + i += functionDescription.nbrArgRepeating - 1; + continue; + } + + // End case: it's a required argument + valueIndexToArgPosition.set(countValueSupplied, i); + countValueSupplied++; + } + + return (argPosition: number) => { + return valueIndexToArgPosition.get(argPosition); }; } @@ -153,10 +243,17 @@ function argTargeting(countArg, repeatingArg): (argPosition: number) => number { // Argument validation //------------------------------------------------------------------------------ -export function validateArguments(args: ArgDefinition[]) { - let previousArgRepeating: boolean | undefined = false; - let previousArgOptional: boolean | undefined = false; - let previousArgDefault: boolean | undefined = false; +export function validateArguments( + args: ArgDefinition[], + nbrArgOptional: number, + nbrArgRepeating: number +) { + if (nbrArgRepeating && nbrArgOptional >= nbrArgRepeating) { + throw new Error(_t("Function ${name} has more optional arguments than repeatable ones.")); + } + + let foundRepeating = false; + let consecutiveRepeating = false; for (let current of args) { if (current.type.includes("META") && current.type.length > 1) { throw new Error( @@ -166,24 +263,18 @@ export function validateArguments(args: ArgDefinition[]) { ); } - if (previousArgRepeating && !current.repeating) { - throw new Error( - _t( - "Function ${name} has no-repeatable arguments declared after repeatable ones. All repeatable arguments must be declared last." - ) - ); - } - const previousIsOptional = previousArgOptional || previousArgRepeating || previousArgDefault; - const currentIsntOptional = !(current.optional || current.repeating || current.default); - if (previousIsOptional && currentIsntOptional) { - throw new Error( - _t( - "Function ${name} has at mandatory arguments declared after optional ones. All optional arguments must be after all mandatory arguments." - ) - ); + if (current.repeating) { + if (!consecutiveRepeating && foundRepeating) { + throw new Error( + _t( + "Function ${name} has non-consecutive repeating arguments. All repeating arguments must be declared consecutively." + ) + ); + } + foundRepeating = true; + consecutiveRepeating = true; + } else { + consecutiveRepeating = false; } - previousArgRepeating = current.repeating; - previousArgOptional = current.optional; - previousArgDefault = current.default; } } diff --git a/src/functions/index.ts b/src/functions/index.ts index 513db3250d..a8e146922e 100644 --- a/src/functions/index.ts +++ b/src/functions/index.ts @@ -12,7 +12,7 @@ import { isMatrix, } from "../types"; import { BadExpressionError, EvaluationError, NotAvailableError } from "../types/errors"; -import { addMetaInfoFromArg, validateArguments } from "./arguments"; +import { addMetaInfoFromArg, argTargeting, validateArguments } from "./arguments"; import { generateMatrix, isEvaluationError, matrixForEach, matrixMap } from "./helpers"; import * as array from "./module_array"; import * as misc from "./module_custom"; @@ -84,7 +84,7 @@ export class FunctionRegistry extends Registry { ); } const descr = addMetaInfoFromArg(addDescr); - validateArguments(descr.args); + validateArguments(descr.args, descr.nbrArgOptional, descr.nbrArgRepeating); this.mapping[name] = createComputeFunction(descr, name); super.add(name, descr); return this; @@ -128,9 +128,11 @@ function createComputeFunction( let vectorArgsType: VectorArgType[] | undefined = undefined; + const getArgToFocus = argTargeting(descr, args.length); //#region Compute vectorisation limits for (let i = 0; i < args.length; i++) { - const argDefinition = descr.args[descr.getArgToFocus(i + 1) - 1]; + const argIndex = getArgToFocus(i) ?? -1; + const argDefinition = descr.args[argIndex]; const arg = args[i]; if (isMatrix(arg) && !argDefinition.acceptMatrix) { @@ -217,7 +219,8 @@ function createComputeFunction( ): Matrix | FunctionResultObject { for (let i = 0; i < args.length; i++) { const arg = args[i]; - const argDefinition = descr.args[descr.getArgToFocus(i + 1) - 1]; + const getArgToFocus = argTargeting(descr, args.length); + const argDefinition = descr.args[getArgToFocus(i) || i]; // Early exit if the argument is an error and the function does not accept errors // We only check scalar arguments, not matrix arguments for performance reasons. diff --git a/src/functions/module_filter.ts b/src/functions/module_filter.ts index cf857fa257..8fc542e857 100644 --- a/src/functions/module_filter.ts +++ b/src/functions/module_filter.ts @@ -200,7 +200,7 @@ export const SORTN: AddFunctionDescription = { description: _t("Returns the first n items in a data set after performing a sort."), args: [ arg("range (range)", _t("The data to be sorted.")), - arg("n (number, default=1)", _t("The number of items to return.")), + arg("n (number)", _t("The number of items to return.")), arg( "display_ties_mode (number, default=0)", _t("A number representing the way to display ties.") @@ -221,12 +221,20 @@ export const SORTN: AddFunctionDescription = { compute: function ( range: Matrix, n: Maybe, - displayTiesMode: Maybe, - ...sortingCriteria: (FunctionResultObject | Matrix)[] + ...displayTiesMode_sortingCriteria: [Maybe, ...Array] ): any { const _n = toNumber(n?.value ?? 1, this.locale); + + const _displayTiesMode: number = + displayTiesMode_sortingCriteria.length % 2 === 0 + ? 0 + : toNumber(displayTiesMode_sortingCriteria[0]?.value, this.locale); + const sortingCriteria: Arg[] = + displayTiesMode_sortingCriteria.length % 2 === 0 + ? displayTiesMode_sortingCriteria + : displayTiesMode_sortingCriteria.slice(1); + assert(() => _n >= 0, _t("Wrong value of 'n'. Expected a positive number. Got %s.", _n)); - const _displayTiesMode = toNumber(displayTiesMode?.value ?? 0, this.locale); assert( () => _displayTiesMode >= 0 && _displayTiesMode <= 3, _t( diff --git a/src/functions/module_lookup.ts b/src/functions/module_lookup.ts index 0ada5f127e..8cbf34bd78 100644 --- a/src/functions/module_lookup.ts +++ b/src/functions/module_lookup.ts @@ -697,8 +697,8 @@ export const PIVOT_VALUE = { args: [ arg("pivot_id (number,string)", _t("ID of the pivot.")), arg("measure_name (string)", _t("Name of the measure.")), - arg("domain_field_name (string,optional,repeating)", _t("Field name.")), - arg("domain_value (number,string,boolean,optional,repeating)", _t("Value.")), + arg("domain_field_name (string,repeating)", _t("Field name.")), + arg("domain_value (number,string,boolean,repeating)", _t("Value.")), ], compute: function ( formulaId: Maybe, @@ -745,8 +745,8 @@ export const PIVOT_HEADER = { description: _t("Get the header of a pivot."), args: [ arg("pivot_id (number,string)", _t("ID of the pivot.")), - arg("domain_field_name (string,optional,repeating)", _t("Field name.")), - arg("domain_value (number,string,value,optional,repeating)", _t("Value.")), + arg("domain_field_name (string,repeating)", _t("Field name.")), + arg("domain_value (number,string,value,repeating)", _t("Value.")), ], compute: function ( pivotId: Maybe, diff --git a/src/types/functions.ts b/src/types/functions.ts index 7f49bb3462..48f0585dd4 100644 --- a/src/types/functions.ts +++ b/src/types/functions.ts @@ -48,7 +48,7 @@ export type FunctionDescription = AddFunctionDescription & { minArgRequired: number; maxArgPossible: number; nbrArgRepeating: number; - getArgToFocus: (argPosition: number) => number; + nbrArgOptional: number; }; export type EvalContext = { diff --git a/tests/composer/formula_assistant_component.test.ts b/tests/composer/formula_assistant_component.test.ts index 666716c2bc..358f4b0411 100644 --- a/tests/composer/formula_assistant_component.test.ts +++ b/tests/composer/formula_assistant_component.test.ts @@ -60,26 +60,53 @@ describe("formula assistant", () => { setTranslationMethod((str, ...values) => str); functionRegistry.add("FUNC2", { description: "func2 def", - args: [ - arg("f2Arg1 (any)", "f2 Arg1 def"), - arg("f2Arg2 (any, optional, default=TRUE)", "f2 Arg2 def"), - ], + args: [arg("f2Arg1 (any)", "f2 Arg1 def"), arg("f2Arg2 (any, default=TRUE)", "f2 Arg2 def")], compute: () => 1, }); functionRegistry.add("FUNC3", { description: "func3 def", - args: [ - arg("f3Arg1 (any)", "f3 Arg1 def"), - arg("f3Arg2 (any, optional, repeating)", "f3 Arg2 def"), - ], + args: [arg("f3Arg1 (any)", "f3 Arg1 def"), arg("f3Arg2 (any, repeating)", "f3 Arg2 def")], compute: () => 1, }); functionRegistry.add("UPTOWNFUNC", { description: "a Bruno Mars song ?", args: [ arg("f4Arg1 (any)", "f4 Arg1 def"), - arg("f4Arg2 (any, optional, repeating)", "f4 Arg2 def"), - arg("f4Arg3 (any, optional, repeating)", "f4 Arg3 def"), + arg("f4Arg2 (any, repeating)", "f4 Arg2 def"), + arg("f4Arg3 (any, repeating)", "f4 Arg3 def"), + ], + compute: () => 1, + }); + functionRegistry.add("FUNC5", { + description: "a function with one optional argument defined after two repeating argument", + args: [ + arg("f5Arg1 (any)", "f5 Arg1 def"), + arg("f5Arg2 (any, repeating)", "f5 Arg2 def"), + arg("f5Arg3 (any, repeating)", "f5 Arg3 def"), + arg("f5Arg4 (any, optional)", "f5 Arg4 def"), + ], + compute: () => 1, + }); + functionRegistry.add("FUNC6", { + description: "a function with one optional argument defined after three repeating arguments", + args: [ + arg("f6Arg1 (any)", "f6 Arg1 def"), + arg("f6Arg2 (any, repeating)", "f6 Arg2 def"), + arg("f6Arg3 (any, repeating)", "f6 Arg3 def"), + arg("f6Arg4 (any, repeating)", "f6 Arg4 def"), + arg("f6Arg5 (any, optional)", "f6 Arg5 def"), + ], + compute: () => 1, + }); + functionRegistry.add("FUNC7", { + description: "a function with two optional arguments defined after three repeating arguments", + args: [ + arg("f7Arg1 (any)", "f7 Arg1 def"), + arg("f7Arg2 (any, repeating)", "f7 Arg2 def"), + arg("f7Arg3 (any, repeating)", "f7 Arg3 def"), + arg("f7Arg4 (any, repeating)", "f7 Arg4 def"), + arg("f7Arg5 (any, optional)", "f7 Arg5 def"), + arg("f7Arg6 (any, optional)", "f7 Arg6 def"), ], compute: () => 1, }); @@ -439,6 +466,234 @@ describe("formula assistant", () => { ).toBe("f4Arg2"); }); }); + + describe("functions with optional argument defined after a repeating argument", () => { + test("=FUNC5(1, 2, focus on 3th argument", async () => { + await typeInComposer("=FUNC5(1, 2,"); + expect( + fixture.querySelectorAll(".o-formula-assistant-arg.o-formula-assistant-focus span")[0] + .textContent + ).toBe("f5Arg3"); + }); + + test("=FUNC5(1, 2, 3, focus on 2nd and 4th arguments", async () => { + await typeInComposer("=FUNC5(1, 2, 3,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(2); + expect(focusArgs[0].textContent).toBe("f5Arg2"); + expect(focusArgs[1].textContent).toBe("f5Arg4"); + }); + + test("=FUNC5(1, 2, 3, 4, focus on 3th argument", async () => { + await typeInComposer("=FUNC5(1, 2, 3, 4,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f5Arg3"); + }); + + test("=FUNC5(1, 2, 3, 4, 5 and comme back on the 4th argument --> focus on the 2nd argument only", async () => { + await typeInComposer("=FUNC5(1, 2, 3, 4, 5"); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f5Arg2"); + }); + }); + + describe("functions with one optional argument defined after two repeating arguments", () => { + test("=FUNC5(1, 2, focus on 3th argument", async () => { + await typeInComposer("=FUNC5(1, 2,"); + expect( + fixture.querySelectorAll(".o-formula-assistant-arg.o-formula-assistant-focus span")[0] + .textContent + ).toBe("f5Arg3"); + }); + + test("=FUNC5(1, 2, 3, focus on 2nd and 4th arguments", async () => { + await typeInComposer("=FUNC5(1, 2, 3,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(2); + expect(focusArgs[0].textContent).toBe("f5Arg2"); + expect(focusArgs[1].textContent).toBe("f5Arg4"); + }); + + test("=FUNC5(1, 2, 3, 4, focus on 3th argument", async () => { + await typeInComposer("=FUNC5(1, 2, 3, 4,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f5Arg3"); + }); + + test("=FUNC5(1, 2, 3, 4, 5 and comme back on the 4th argument --> focus on the 2nd argument only", async () => { + await typeInComposer("=FUNC5(1, 2, 3, 4, 5"); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f5Arg2"); + }); + }); + + describe("functions with one optional argument defined after three repeating arguments", () => { + test("=FUNC6(1, 2, 3, 4, focus on 2nd and 5th argument", async () => { + await typeInComposer("=FUNC6(1, 2, 3, 4,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(2); + expect(focusArgs[0].textContent).toBe("f6Arg2"); + expect(focusArgs[1].textContent).toBe("f6Arg5"); + }); + + test("=FUNC6(1, 2, 3, 4, 5, focus on 3th arguments", async () => { + await typeInComposer("=FUNC6(1, 2, 3, 4, 5,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f6Arg3"); + }); + + test("=FUNC6(1, 2, 3, 4, 5, 6, focus on 4th arguments", async () => { + await typeInComposer("=FUNC6(1, 2, 3, 4, 5, 6,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f6Arg4"); + }); + + test("=FUNC6(1, 2, 3, 4, 5, 6, and comme back on previous argument --> focus only one argument", async () => { + await typeInComposer("=FUNC6(1, 2, 3, 4, 5, 6, 7"); + + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + let focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f6Arg3"); + + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f6Arg2"); + }); + }); + + describe("functions with two optional arguments defined after three repeating arguments", () => { + test("=FUNC7(1, 2, 3, 4, focus on 2nd and 5th argument", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(2); + expect(focusArgs[0].textContent).toBe("f7Arg2"); + expect(focusArgs[1].textContent).toBe("f7Arg5"); + }); + + test("=FUNC7(1, 2, 3, 4, 5, focus on 3th and 6th arguments", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4, 5,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(2); + expect(focusArgs[0].textContent).toBe("f7Arg3"); + expect(focusArgs[1].textContent).toBe("f7Arg6"); + }); + + test("=FUNC7(1, 2, 3, 4, 5, 6, focus on 4th arguments", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4, 5, 6,"); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f7Arg4"); + }); + + test("=FUNC7(1, 2, 3, 4, 5, 6, 7 and comme back on previous argument --> focus only one argument", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4, 5, 6, 7"); + + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + let focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f7Arg3"); + + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f7Arg2"); + }); + }); + + describe("function with right parenthesis --> freeze args count and focus maximum one arg", () => { + test("type =FUNC6(1, 2, 3, 4, ) and move one the 5th position --> focus the 5th argument only", async () => { + await typeInComposer("=FUNC6(1, 2, 3, 4, )"); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f6Arg5"); + }); + + test("type =FUNC7(1, 2, 3, 4, ) and move on the 5th position --> focus the 5th argument only", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4, )"); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f7Arg5"); + }); + + test("type =FUNC7(1, 2, 3, 4, 5, ) and move on the 6th position --> focus the 6th argument only", async () => { + await typeInComposer("=FUNC7(1, 2, 3, 4, 5, )"); + await keyDown({ key: "ArrowLeft" }); + await keyUp({ key: "ArrowLeft" }); + const focusArgs = fixture.querySelectorAll( + ".o-formula-assistant-arg.o-formula-assistant-focus div span:first-child" + ); + expect(focusArgs.length).toBe(1); + expect(focusArgs[0].textContent).toBe("f7Arg6"); + }); + }); }); }); diff --git a/tests/evaluation/compiler.test.ts b/tests/evaluation/compiler.test.ts index 06ebbe2850..db11be7dba 100644 --- a/tests/evaluation/compiler.test.ts +++ b/tests/evaluation/compiler.test.ts @@ -141,7 +141,7 @@ describe("compile functions", () => { }, args: [ { name: "arg1", description: "", type: ["ANY"] }, - { name: "arg2", description: "", type: ["ANY"], optional: true, repeating: true }, + { name: "arg2", description: "", type: ["ANY"], repeating: true }, ], }); expect(compiledBaseFunction("=REPEATABLE(1)").isBadExpression).toBe(false); @@ -158,8 +158,8 @@ describe("compile functions", () => { }, args: [ { name: "arg1", description: "", type: ["ANY"] }, - { name: "arg2", description: "", type: ["ANY"], optional: true, repeating: true }, - { name: "arg3", description: "", type: ["ANY"], optional: true, repeating: true }, + { name: "arg2", description: "", type: ["ANY"], repeating: true }, + { name: "arg3", description: "", type: ["ANY"], repeating: true }, ], }); expect(compiledBaseFunction("=REPEATABLES(1, 2)").isBadExpression).toBe(true); diff --git a/tests/functions/arguments.test.ts b/tests/functions/arguments.test.ts index 01691b15bf..de35be7d5a 100644 --- a/tests/functions/arguments.test.ts +++ b/tests/functions/arguments.test.ts @@ -1,4 +1,9 @@ -import { addMetaInfoFromArg, arg, validateArguments } from "../../src/functions/arguments"; +import { + addMetaInfoFromArg, + arg, + argTargeting, + validateArguments, +} from "../../src/functions/arguments"; import { AddFunctionDescription } from "../../src/types"; describe("args", () => { @@ -122,58 +127,75 @@ describe("args", () => { describe("arguments validation", () => { test("'META' type can only be declared alone", () => { - expect(() => validateArguments([arg("metaArg (meta)")])).not.toThrow(); - expect(() => validateArguments([arg("metaArg (meta, optional)")])).not.toThrow(); - expect(() => validateArguments([arg("metaArg (meta, repeating)")])).not.toThrow(); - - expect(() => validateArguments([arg("metaArg (meta, any)")])).toThrow(); - expect(() => validateArguments([arg("metaArg (meta, range)")])).toThrow(); - expect(() => validateArguments([arg("metaArg (meta, number)")])).toThrow(); - expect(() => validateArguments([arg("metaArg (meta, string)")])).toThrow(); - expect(() => validateArguments([arg("metaArg (meta, boolean)")])).toThrow(); + expect(() => validateArguments([arg("metaArg (meta)")], 0, 0)).not.toThrow(); + expect(() => validateArguments([arg("metaArg (meta, optional)")], 1, 0)).not.toThrow(); + expect(() => validateArguments([arg("metaArg (meta, repeating)")], 0, 1)).not.toThrow(); + + expect(() => validateArguments([arg("metaArg (meta, any)")], 0, 0)).toThrow(); + expect(() => validateArguments([arg("metaArg (meta, range)")], 0, 0)).toThrow(); + expect(() => validateArguments([arg("metaArg (meta, number)")], 0, 0)).toThrow(); + expect(() => validateArguments([arg("metaArg (meta, string)")], 0, 0)).toThrow(); + expect(() => validateArguments([arg("metaArg (meta, boolean)")], 0, 0)).toThrow(); }); - test("All repeatable arguments must be declared last", () => { + test("All repeatable arguments must be declared consecutively", () => { + expect(() => + validateArguments([arg("arg1 (any)"), arg("arg2 (any, repeating)")], 0, 1) + ).not.toThrow(); expect(() => - validateArguments([arg("arg1 (any)"), arg("arg2 (any, repeating)")]) + validateArguments( + [arg("arg1 (any)"), arg("arg2 (any, repeating)"), arg("arg3 (any, repeating)")], + 0, + 2 + ) ).not.toThrow(); expect(() => - validateArguments([ - arg("arg1 (any)"), - arg("arg2 (any, repeating)"), - arg("arg3 (any, repeating)"), - ]) + validateArguments([arg("arg1 (any)"), arg("arg2 (any, repeating)"), arg("arg3 (any)")], 0, 1) ).not.toThrow(); expect(() => - validateArguments([arg("arg1 (any)"), arg("arg2 (any, repeating)"), arg("arg3 (any)")]) + validateArguments( + [ + arg("arg1 (any)"), + arg("arg2 (any, repeating)"), + arg("arg3 (any, optional)"), + arg("arg4 (any, repeating)"), + ], + 1, + 2 + ) ).toThrow(); expect(() => - validateArguments([ - arg("arg1 (any)"), - arg("arg2 (any, repeating)"), - arg("arg3 (any, optional)"), - ]) + validateArguments( + [arg("arg1 (any, repeating)"), arg("arg2 (any)"), arg("arg3 (any, repeating)")], + 0, + 2 + ) ).toThrow(); }); - test("All optional arguments must be after all mandatory arguments", () => { - expect(() => - validateArguments([ - arg("arg1 (any)"), - arg("arg2 (any, optional)"), - arg("arg3 (any, optional)"), - ]) - ).not.toThrow(); + test("If repeatable arguments --> The number of repeatable arguments must be greater than the number of optional arguments", () => { expect(() => - validateArguments([ - arg("arg1 (any)"), - arg("arg2 (any, optional)"), - arg("arg3 (any, repeating)"), - ]) + validateArguments([arg("arg1 (any)"), arg("arg2 (any, optional)")], 1, 0) ).not.toThrow(); expect(() => - validateArguments([arg("arg1 (any)"), arg("arg2 (any, optional)"), arg("arg3 (any)")]) + validateArguments( + [arg("arg1 (any)"), arg("arg2 (any, optional)"), arg("arg3 (any, repeating)")], + 1, + 1 + ) ).toThrow(); + expect(() => + validateArguments( + [ + arg("arg1 (any)"), + arg("arg2 (any, optional)"), + arg("arg3 (any, repeating)"), + arg("arg4 (any, repeating)"), + ], + 1, + 2 + ) + ).not.toThrow(); }); }); @@ -194,12 +216,11 @@ describe("function addMetaInfoFromArg", () => { expect(descr.minArgRequired).toBe(2); expect(descr.maxArgPossible).toBe(2); expect(descr.nbrArgRepeating).toBe(0); + expect(descr.nbrArgOptional).toBe(0); - const getArgToFocus = descr.getArgToFocus!; - expect(getArgToFocus(-1)).toBe(-1); + const getArgToFocus = argTargeting(descr, 2); + expect(getArgToFocus(0)).toBe(0); expect(getArgToFocus(1)).toBe(1); - expect(getArgToFocus(2)).toBe(2); - expect(getArgToFocus(42)).toBe(42); }); test("with optional arguments", () => { @@ -218,12 +239,14 @@ describe("function addMetaInfoFromArg", () => { expect(descr.minArgRequired).toBe(1); expect(descr.maxArgPossible).toBe(2); expect(descr.nbrArgRepeating).toBe(0); + expect(descr.nbrArgOptional).toBe(1); - const getArgToFocus = descr.getArgToFocus!; - expect(getArgToFocus(-1)).toBe(-1); - expect(getArgToFocus(1)).toBe(1); - expect(getArgToFocus(2)).toBe(2); - expect(getArgToFocus(42)).toBe(42); + const getArgToFocusOnOneArg = argTargeting(descr, 1); + expect(getArgToFocusOnOneArg(0)).toBe(0); + + const getArgToFocusOnTwoArgs = argTargeting(descr, 2); + expect(getArgToFocusOnTwoArgs(0)).toBe(0); + expect(getArgToFocusOnTwoArgs(1)).toBe(1); }); test("with repeatable argument", () => { @@ -242,12 +265,16 @@ describe("function addMetaInfoFromArg", () => { expect(descr.minArgRequired).toBe(1); expect(descr.maxArgPossible).toBe(Infinity); expect(descr.nbrArgRepeating).toBe(1); + expect(descr.nbrArgOptional).toBe(0); - const getArgToFocus = descr.getArgToFocus!; - expect(getArgToFocus(-1)).toBe(-1); - expect(getArgToFocus(1)).toBe(1); - expect(getArgToFocus(2)).toBe(2); - expect(getArgToFocus(42)).toBe(2); + const getArgToFocusOnOneArg = argTargeting(descr, 1); + expect(getArgToFocusOnOneArg(0)).toBe(0); + + const getArgToFocusOnSeveralArgs = argTargeting(descr, 42); + expect(getArgToFocusOnSeveralArgs(0)).toBe(0); + expect(getArgToFocusOnSeveralArgs(1)).toBe(1); + expect(getArgToFocusOnSeveralArgs(20)).toBe(1); + expect(getArgToFocusOnSeveralArgs(41)).toBe(1); }); test("with more than one repeatable argument", () => { @@ -267,13 +294,240 @@ describe("function addMetaInfoFromArg", () => { expect(descr.minArgRequired).toBe(1); expect(descr.maxArgPossible).toBe(Infinity); expect(descr.nbrArgRepeating).toBe(2); + expect(descr.nbrArgOptional).toBe(0); - const getArgToFocus = descr.getArgToFocus!; - expect(getArgToFocus(-1)).toBe(-1); + const getArgToFocus = argTargeting(descr, 42); + expect(getArgToFocus(0)).toBe(0); expect(getArgToFocus(1)).toBe(1); expect(getArgToFocus(2)).toBe(2); - expect(getArgToFocus(3)).toBe(3); - expect(getArgToFocus(5)).toBe(3); - expect(getArgToFocus(8)).toBe(2); + expect(getArgToFocus(4)).toBe(2); + expect(getArgToFocus(7)).toBe(1); + }); + + test("with optional arg after repeatable argument", () => { + // like the SWITCH function + const useRepeatables = { + description: "function with many repeatable argument", + compute: (arg) => { + return true; + }, + args: [ + { name: "arg1", description: "", type: ["ANY"] }, + { name: "arg2", description: "", type: ["ANY"], repeating: true }, + { name: "arg3", description: "", type: ["ANY"], repeating: true }, + { name: "arg4", description: "", type: ["ANY"], optional: true }, + ], + } as AddFunctionDescription; + + const descr = addMetaInfoFromArg(useRepeatables); + expect(descr.minArgRequired).toBe(1); + expect(descr.maxArgPossible).toBe(Infinity); + expect(descr.nbrArgRepeating).toBe(2); + expect(descr.nbrArgOptional).toBe(1); + + const getArgToFocus_1 = argTargeting(descr, 1); + expect(getArgToFocus_1(0)).toBe(0); + + const getArgToFocus_3 = argTargeting(descr, 3); + expect(getArgToFocus_3(0)).toBe(0); + expect(getArgToFocus_3(1)).toBe(1); + expect(getArgToFocus_3(2)).toBe(2); + + const getArgToFocus_4 = argTargeting(descr, 4); + expect(getArgToFocus_4(0)).toBe(0); + expect(getArgToFocus_4(1)).toBe(1); + expect(getArgToFocus_4(2)).toBe(2); + expect(getArgToFocus_4(3)).toBe(3); + + const getArgToFocus_5 = argTargeting(descr, 5); + expect(getArgToFocus_5(0)).toBe(0); + expect(getArgToFocus_5(1)).toBe(1); + expect(getArgToFocus_5(2)).toBe(2); + expect(getArgToFocus_5(3)).toBe(1); + expect(getArgToFocus_5(4)).toBe(2); + }); + + test("with 2 optionals arg after 3 repeatable arguments", () => { + // like the SWITCH function + const useRepeatables = { + description: "function with many repeatable argument", + compute: (arg) => { + return true; + }, + args: [ + { name: "arg1", description: "", type: ["ANY"] }, + { name: "arg2", description: "", type: ["ANY"], repeating: true }, + { name: "arg3", description: "", type: ["ANY"], repeating: true }, + { name: "arg4", description: "", type: ["ANY"], repeating: true }, + { name: "arg5", description: "", type: ["ANY"], optional: true }, + { name: "arg6", description: "", type: ["ANY"], optional: true }, + ], + } as AddFunctionDescription; + + const descr = addMetaInfoFromArg(useRepeatables); + expect(descr.minArgRequired).toBe(1); + expect(descr.maxArgPossible).toBe(Infinity); + expect(descr.nbrArgRepeating).toBe(3); + expect(descr.nbrArgOptional).toBe(2); + + const getArgToFocus_1 = argTargeting(descr, 1); + expect(getArgToFocus_1(0)).toBe(0); + + const getArgToFocus_5 = argTargeting(descr, 5); + expect(getArgToFocus_5(0)).toBe(0); + expect(getArgToFocus_5(1)).toBe(1); + expect(getArgToFocus_5(2)).toBe(2); + expect(getArgToFocus_5(3)).toBe(3); + expect(getArgToFocus_5(4)).toBe(4); + + const getArgToFocus_8 = argTargeting(descr, 8); + expect(getArgToFocus_8(0)).toBe(0); + expect(getArgToFocus_8(1)).toBe(1); + expect(getArgToFocus_8(2)).toBe(2); + expect(getArgToFocus_8(3)).toBe(3); + expect(getArgToFocus_8(4)).toBe(1); + expect(getArgToFocus_8(5)).toBe(2); + expect(getArgToFocus_8(6)).toBe(3); + expect(getArgToFocus_8(7)).toBe(4); + }); + + test("with required arg after repeatable argument", () => { + // like the SWITCH function + const useRepeatables = { + description: "function with many repeatable argument", + compute: (arg) => { + return true; + }, + args: [ + { name: "arg1", description: "", type: ["ANY"] }, + { name: "arg2", description: "", type: ["ANY"], repeating: true }, + { name: "arg3", description: "", type: ["ANY"], repeating: true }, + { name: "arg4", description: "", type: ["ANY"] }, + ], + } as AddFunctionDescription; + + const descr = addMetaInfoFromArg(useRepeatables); + expect(descr.minArgRequired).toBe(2); + expect(descr.maxArgPossible).toBe(Infinity); + expect(descr.nbrArgRepeating).toBe(2); + expect(descr.nbrArgOptional).toBe(0); + + const getArgToFocus_2 = argTargeting(descr, 2); + expect(getArgToFocus_2(0)).toBe(0); + expect(getArgToFocus_2(1)).toBe(3); + + const getArgToFocus_4 = argTargeting(descr, 4); + expect(getArgToFocus_4(0)).toBe(0); + expect(getArgToFocus_4(1)).toBe(1); + expect(getArgToFocus_4(2)).toBe(2); + expect(getArgToFocus_4(3)).toBe(3); + + const getArgToFocus_6 = argTargeting(descr, 6); + expect(getArgToFocus_6(0)).toBe(0); + expect(getArgToFocus_6(1)).toBe(1); + expect(getArgToFocus_6(2)).toBe(2); + expect(getArgToFocus_6(3)).toBe(1); + expect(getArgToFocus_6(4)).toBe(2); + expect(getArgToFocus_6(5)).toBe(3); + }); + + test("with required arg after optional argument", () => { + // like the SWITCH function + const useRepeatables = { + description: "function with many repeatable argument", + compute: (arg) => { + return true; + }, + args: [ + { name: "arg1", description: "", type: ["ANY"] }, + { name: "arg2", description: "", type: ["ANY"], optional: true }, + { name: "arg3", description: "", type: ["ANY"], optional: true }, + { name: "arg4", description: "", type: ["ANY"] }, + ], + } as AddFunctionDescription; + + const descr = addMetaInfoFromArg(useRepeatables); + expect(descr.minArgRequired).toBe(2); + expect(descr.maxArgPossible).toBe(4); + expect(descr.nbrArgRepeating).toBe(0); + expect(descr.nbrArgOptional).toBe(2); + + const getArgToFocus_2 = argTargeting(descr, 2); + expect(getArgToFocus_2(0)).toBe(0); + expect(getArgToFocus_2(1)).toBe(3); + + const getArgToFocus_3 = argTargeting(descr, 3); + expect(getArgToFocus_3(0)).toBe(0); + expect(getArgToFocus_3(1)).toBe(1); + expect(getArgToFocus_3(2)).toBe(3); + + const getArgToFocus_4 = argTargeting(descr, 4); + expect(getArgToFocus_4(0)).toBe(0); + expect(getArgToFocus_4(1)).toBe(1); + expect(getArgToFocus_4(2)).toBe(2); + expect(getArgToFocus_4(3)).toBe(3); + }); + + test("a random case", () => { + // like the SWITCH function + const useRepeatables = { + description: "function with many repeatable argument", + compute: (arg) => { + return true; + }, + args: [ + { name: "arg1", description: "", type: ["ANY"], optional: true }, + { name: "arg2", description: "", type: ["ANY"] }, + { name: "arg3", description: "", type: ["ANY"], optional: true }, + { name: "arg4", description: "", type: ["ANY"] }, + { name: "arg5", description: "", type: ["ANY"], repeating: true }, + { name: "arg6", description: "", type: ["ANY"], repeating: true }, + { name: "arg7", description: "", type: ["ANY"], repeating: true }, + { name: "arg8", description: "", type: ["ANY"], repeating: true }, + { name: "arg9", description: "", type: ["ANY"], optional: true }, + { name: "arg10", description: "", type: ["ANY"] }, + ], + } as AddFunctionDescription; + + const descr = addMetaInfoFromArg(useRepeatables); + expect(descr.minArgRequired).toBe(3); + expect(descr.maxArgPossible).toBe(Infinity); + expect(descr.nbrArgRepeating).toBe(4); + expect(descr.nbrArgOptional).toBe(3); + + const getArgToFocus_3 = argTargeting(descr, 3); + expect(getArgToFocus_3(0)).toBe(1); + expect(getArgToFocus_3(1)).toBe(3); + expect(getArgToFocus_3(2)).toBe(9); + + const getArgToFocus_4 = argTargeting(descr, 4); + expect(getArgToFocus_4(0)).toBe(0); + expect(getArgToFocus_4(1)).toBe(1); + expect(getArgToFocus_4(2)).toBe(3); + expect(getArgToFocus_4(3)).toBe(9); + + const getArgToFocus_5 = argTargeting(descr, 5); + expect(getArgToFocus_5(0)).toBe(0); + expect(getArgToFocus_5(1)).toBe(1); + expect(getArgToFocus_5(2)).toBe(2); + expect(getArgToFocus_5(3)).toBe(3); + expect(getArgToFocus_5(4)).toBe(9); + + const getArgToFocus_6 = argTargeting(descr, 6); + expect(getArgToFocus_6(0)).toBe(0); + expect(getArgToFocus_6(1)).toBe(1); + expect(getArgToFocus_6(2)).toBe(2); + expect(getArgToFocus_6(3)).toBe(3); + expect(getArgToFocus_6(4)).toBe(8); + expect(getArgToFocus_6(5)).toBe(9); + + const getArgToFocus_7 = argTargeting(descr, 7); + expect(getArgToFocus_7(0)).toBe(1); + expect(getArgToFocus_7(1)).toBe(3); + expect(getArgToFocus_7(2)).toBe(4); + expect(getArgToFocus_7(3)).toBe(5); + expect(getArgToFocus_7(4)).toBe(6); + expect(getArgToFocus_7(5)).toBe(7); + expect(getArgToFocus_7(6)).toBe(9); }); }); diff --git a/tests/functions/module_filter.test.ts b/tests/functions/module_filter.test.ts index fad5276532..81f5f462ad 100644 --- a/tests/functions/module_filter.test.ts +++ b/tests/functions/module_filter.test.ts @@ -852,26 +852,18 @@ describe("SORTN function", () => { ]); }); - test("Full sorting with multiple columns using default parameters", () => { - //prettier-ignore + test("Accept at least two arguments", () => { + // @compatibility: Google Sheets requires at least one argument only. + // Here, depending on the implementation, they couldn't have more (or equal) optional arguments than repeatable arguments. const grid = { - A1: "1", B1: "1", C1: "1", - A2: "2", B2: "1", C2: "2", - A3: "3", B3: "2", C3: "1", - A4: "1", B4: "3", C4: "3", - A5: "2", B5: "3", C5: "1", - A6: "4", B6: "2", C6: "1", - A7: "3", B7: "4", C7: "4", - A8: "2", B8: "1", C8: "1", - A9: "4", B9: "1", C9: "2", - A10: "2", B10: "1", C10: "1", + A1: "1", + A2: "2", + A3: "3", + A4: "1", + A5: "2", + A6: "2", }; - const model = createModelFromGrid(grid); - setCellContent(model, "A11", "=SORTN(A1:C10)"); - expect(getRangeValuesAsMatrix(model, "A11:C12")).toEqual([ - [1, 1, 1], - [null, null, null], - ]); + expect(evaluateCell("A7", { A7: "=SORTN(A1:A6)", ...grid })).toBe("#BAD_EXPR"); }); test("Full sorting with different display ties modes", () => { @@ -919,6 +911,32 @@ describe("SORTN function", () => { ]); }); + test("When 4 values supplied, the 3th value and 4th value correspond to the 4th and 5th arguments", () => { + // see 'argTargeting' to understand how the arguments are targeted + //prettier-ignore + const grid = { + A1: "1", B1: "1", C1: "1", + A2: "2", B2: "1", C2: "2", + A3: "3", B3: "2", C3: "1", + A4: "1", B4: "3", C4: "3", + A5: "2", B5: "3", C5: "1", + A6: "2", B6: "3", C6: "1", + A7: "3", B7: "4", C7: "4", + A8: "2", B8: "1", C8: "1", + A9: "4", B9: "1", C9: "2", + A10: "2", B10: "1", C10: "1", + }; + const model = createModelFromGrid(grid); + setCellContent(model, "A11", "=SORTN(A1:C10, 5, 2, true)"); + expect(getRangeValuesAsMatrix(model, "A11:C15")).toEqual([ + [1, 1, 1], + [2, 1, 2], + [2, 1, 1], + [4, 1, 2], + [2, 1, 1], + ]); + }); + test("Sorting with bad n argument", () => { const model = new Model(); setCellContent(model, "A11", "=SORTN(A1:C10, -1, 0)"); diff --git a/tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts b/tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts index c382e94d00..3404d34e0c 100644 --- a/tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts +++ b/tests/pivots/spreadsheet_pivot/spreadsheet_pivot.test.ts @@ -1248,7 +1248,7 @@ describe("Spreadsheet Pivot", () => { // missing header value setCellContent(model, "A31", '=PIVOT.HEADER(1, "Date:month_number")'); expect(getEvaluatedCell(model, "A31").message).toBe( - "Invalid number of arguments for the PIVOT.HEADER function. Expected all arguments after position 1 to be supplied by groups of 2 arguments" + "Invalid number of arguments for the PIVOT.HEADER function. Repeatable arguments are expected to be supplied by groups of 2 argument(s) with maximum 0 optional argument(s), but got 1 argument(s) too many." ); // without granularity From 92b61aa8e2539cb212b1e364ad66fdf1ff79bb9f Mon Sep 17 00:00:00 2001 From: Alexis Lacroix Date: Fri, 10 Jan 2025 13:31:56 +0100 Subject: [PATCH 2/2] [IMP] formulas: add the SWITCH function Task: 3829128 --- src/functions/helpers.ts | 7 +++ src/functions/module_logical.ts | 55 +++++++++++++++++++++++- src/functions/module_lookup.ts | 8 +--- tests/functions/module_logical.test.ts | 59 ++++++++++++++++++++++++++ 4 files changed, 121 insertions(+), 8 deletions(-) diff --git a/src/functions/helpers.ts b/src/functions/helpers.ts index 8db38a6159..a343d2c8ae 100644 --- a/src/functions/helpers.ts +++ b/src/functions/helpers.ts @@ -39,6 +39,13 @@ export function isEvaluationError(error: Maybe): error is string { return typeof error === "string" && errorTypes.has(error); } +export function valueNotAvailable(searchKey: Maybe): FunctionResultObject { + return { + value: CellErrorType.NotAvailable, + message: _t("Did not find value '%s' in [[FUNCTION_NAME]] evaluation.", toString(searchKey)), + }; +} + // ----------------------------------------------------------------------------- // FORMAT FUNCTIONS // ----------------------------------------------------------------------------- diff --git a/src/functions/module_logical.ts b/src/functions/module_logical.ts index 36afa024b2..53fbd697af 100644 --- a/src/functions/module_logical.ts +++ b/src/functions/module_logical.ts @@ -3,7 +3,13 @@ import { AddFunctionDescription, Arg, FunctionResultObject, Maybe } from "../typ import { CellErrorType, EvaluationError } from "../types/errors"; import { arg } from "./arguments"; import { boolAnd, boolOr } from "./helper_logical"; -import { assert, conditionalVisitBoolean, isEvaluationError, toBoolean } from "./helpers"; +import { + assert, + conditionalVisitBoolean, + isEvaluationError, + toBoolean, + valueNotAvailable, +} from "./helpers"; // ----------------------------------------------------------------------------- // AND @@ -221,6 +227,53 @@ export const OR = { isExported: true, } satisfies AddFunctionDescription; +// ----------------------------------------------------------------------------- +// SWITCH +// ----------------------------------------------------------------------------- + +export const SWITCH = { + description: _t("Returns a value by comparing cases to an expression."), + args: [ + arg("expression (number, boolean, string)", _t("The value to be checked.")), + arg("case1 (number, boolean, string)", _t("The first case to be checked again expression.")), + arg("value1 (any)", _t("The corresponding value to be returned if case1 matches expression.")), + arg( + "case2 (any, repeating)", + _t("Additional cases to try if the previous ones don't match expression.") + ), + arg( + "value2 (any, repeating)", + _t("Additional values to be returned if their corresponding cases match expression.") + ), + arg( + `default (any, default="empty")`, + _t("An optional default value to be returned if none of the cases match expression.") + ), + ], + compute: function ( + expression: Maybe, + ...casesAndValues: Maybe[] + ): FunctionResultObject { + const defaultValue = + casesAndValues.length % 2 === 0 ? valueNotAvailable(expression) : casesAndValues.pop(); + + for (let i = 0; i < casesAndValues.length; i += 2) { + const iCase = casesAndValues[i]; + + if (iCase && isEvaluationError(iCase.value)) { + return iCase; + } + + if (expression?.value === iCase?.value) { + return casesAndValues[i + 1] || { value: 0 }; + } + } + + return defaultValue || { value: 0 }; + }, + isExported: true, +} satisfies AddFunctionDescription; + // ----------------------------------------------------------------------------- // TRUE // ----------------------------------------------------------------------------- diff --git a/src/functions/module_lookup.ts b/src/functions/module_lookup.ts index 8cbf34bd78..a48d1f3d54 100644 --- a/src/functions/module_lookup.ts +++ b/src/functions/module_lookup.ts @@ -25,6 +25,7 @@ import { toMatrix, toNumber, toString, + valueNotAvailable, } from "./helpers"; const DEFAULT_IS_SORTED = true; @@ -32,13 +33,6 @@ const DEFAULT_MATCH_MODE = 0; const DEFAULT_SEARCH_MODE = 1; const DEFAULT_ABSOLUTE_RELATIVE_MODE = 1; -function valueNotAvailable(searchKey: Maybe): FunctionResultObject { - return { - value: CellErrorType.NotAvailable, - message: _t("Did not find value '%s' in [[FUNCTION_NAME]] evaluation.", toString(searchKey)), - }; -} - // ----------------------------------------------------------------------------- // ADDRESS // ----------------------------------------------------------------------------- diff --git a/tests/functions/module_logical.test.ts b/tests/functions/module_logical.test.ts index f9bcd660df..678f254e0c 100644 --- a/tests/functions/module_logical.test.ts +++ b/tests/functions/module_logical.test.ts @@ -408,6 +408,65 @@ describe("OR formula", () => { }); }); +describe("SWITCH formula", () => { + test("take 3 argument minimum", () => { + expect(evaluateCell("A1", { A1: "=SWITCH(42, 42, 42)" })).toBe(42); + expect(evaluateCell("A1", { A1: "=SWITCH(42, 42)" })).toBe("#BAD_EXPR"); + expect(evaluateCell("A1", { A1: "=SWITCH(42)" })).toBe("#BAD_EXPR"); + expect(evaluateCell("A1", { A1: "=SWITCH()" })).toBe("#BAD_EXPR"); + }); + + test("functional tests on simple arguments", () => { + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 123)" })).toBe("#N/A"); + expect(evaluateCell("A1", { A1: "=SWITCH( 23, 23, 123)" })).toBe(123); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 123, 42, 321)" })).toBe(321); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 123, 24, 321)" })).toBe("#N/A"); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 123, 24, 321, 42, 1111)" })).toBe(1111); + + expect(evaluateCell("A1", { A1: "=SWITCH(TRUE, FALSE, 23)" })).toBe("#N/A"); + expect(evaluateCell("A1", { A1: "=SWITCH(TRUE, TRUE, 23)" })).toBe(23); + expect(evaluateCell("A1", { A1: "=SWITCH(23, 23, TRUE)" })).toBe(true); + + expect(evaluateCell("A1", { A1: '=SWITCH("str", 23, 1, "str2", 2, "str", 3)' })).toBe(3); + expect(evaluateCell("A1", { A1: '=SWITCH(25, 26, "str2", 25, "str")' })).toBe("str"); + + expect(evaluateCell("A1", { A1: "=SWITCH( , , 42)" })).toBe(42); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 42, )" })).toBe(0); + + expect(evaluateCell("A1", { A1: '=SWITCH("", "", 42)' })).toBe(42); + expect(evaluateCell("A1", { A1: '=SWITCH(42, 42, "")' })).toBe(""); + + expect(evaluateCell("A1", { A1: '=SWITCH("true" , TRUE, 42)' })).toBe("#N/A"); + expect(evaluateCell("A1", { A1: "=SWITCH(1, TRUE, 42)" })).toBe("#N/A"); + }); + + test("functional with optional value after repeatable values", () => { + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 23, 123)" })).toBe(123); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 23, 42, 321, 123)" })).toBe(321); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 23, 23, 45, 321, )" })).toBe(0); + expect(evaluateCell("A1", { A1: "=SWITCH( 42, 43, 43, )" })).toBe(0); + }); + + test("functional tests on cell arguments", () => { + expect( + evaluateCell("A1", { A1: "=SWITCH(A2, A3, A4)", A2: "test", A3: "TRUE", A4: "42" }) + ).toBe("#N/A"); + expect( + evaluateCell("A1", { A1: "=SWITCH(A2, A3, A4)", A2: "test", A3: "test", A4: "42" }) + ).toBe(42); + }); + + test("return error only when arriving on an error value", () => { + expect(evaluateCell("A1", { A1: "=SWITCH(KABOUM, 42, 42)" })).toBe("#BAD_EXPR"); + expect(evaluateCell("A1", { A1: "=SWITCH(42, KABOUM, 42)" })).toBe("#BAD_EXPR"); + expect(evaluateCell("A1", { A1: "=SWITCH(42, 23, KABOUM, 42, 111)" })).toBe(111); + expect(evaluateCell("A1", { A1: "=SWITCH(42, 42, 111, KABOUM, 666)" })).toBe(111); + expect(evaluateCell("A1", { A1: "=SWITCH(42, 23, KABOUM, KABOUM, 666, 42, 11)" })).toBe( + "#BAD_EXPR" + ); + }); +}); + describe("TRUE formula", () => { test("does not accept argument", () => { expect(evaluateCell("A1", { A1: "=TRUE(45)" })).toBe("#BAD_EXPR"); // @compatibility: on google sheets, return #N/A