[IMP] evaluation: functions can handle errors #2932

laa-odoo · 2023-09-25T14:25:24Z

With those commits, functions can handle errors more easily. See individual commits for details.

The purpose of this commit is to allow the formulas
to return errors as results.

Functionally we want formulas like TRANSPOSE to be
able to transpose an error without invalidating the
entire evaluation process.

We also want formulas like LOOKUP not to be interrupted
when the search range has one or more errors.

Task: 2969401

robodoo · 2023-09-25T14:26:22Z

Pull request status dashboard.

src/functions/index.ts

src/functions/module_info.ts

src/helpers/cells/cell_evaluation.ts

src/plugins/core/cell.ts

src/plugins/ui_stateful/selection.ts

tests/test_helpers/getters_helpers.ts

tests/test_helpers/helpers.ts

hokolomopo

Some small problems and nitpicking.

Really nice work testing every function that may or may not accept error, I certenly wouldn't have bothered testing everything 😛

src/formulas/code_builder.ts

src/functions/helpers.ts

hokolomopo · 2023-12-05T10:30:34Z

src/functions/index.ts

@@ -74,7 +75,7 @@ class FunctionRegistry extends Registry<FunctionDescription> {
    }
    const descr = addMetaInfoFromArg(addDescr);
    validateArguments(descr.args);
-    this.mapping[name] = createComputeFunctionFromDescription(descr);
+    this.mapping[name] = addErrorHandling(createComputeFunctionFromDescription(descr), name);


might as well fo the try/catch directly in createComputeFunctionFromDescription to avoid creating a second function wrapping the first

Indeed, to avoid that, we can put the tryCatch directly in buildComputeFunctionFromDescription. @LucasLefevre are you agree with that ?

src/functions/helpers.ts

src/functions/module_array.ts

src/functions/module_statistical.ts

hokolomopo · 2023-12-05T10:51:03Z

src/helpers/cells/cell_evaluation.ts

-        return evaluateCell;
-      }
+function _createEvaluatedCell(value: CellValue, localeFormat: LocaleFormat): EvaluatedCell {
+  if (value instanceof EvaluationError) {


I'm didn't really liked the builders so I don't really care, but what's the rationale to remove them and to use simple if ? Because that's more readable, or is there a practical reason ?

I find it easier to read, less code to have in mind.
Actually, if it really bothers you, I'll go back.

src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts

tests/evaluation/evaluation.test.ts

LucasLefevre

Nice !
I think we finally found a good way ! Good job :)

Some general comment (more detailed in individual comments)

if we find a more meaningful name for UnitData, it would be nice
throwing objects (and not Errors) makes me feel 🤨 but 🤷
I think there's some useless changes that could be reverted.
Now that the functional side is working with this PR, a next step is to improve performance by using returns as much as possible (especially for ODOO.* functions)

src/functions/module_array.ts

src/functions/helpers.ts

src/types/errors.ts

LucasLefevre · 2023-12-15T10:42:39Z

src/types/misc.ts

@@ -181,13 +179,11 @@ export interface RangeCompiledFormula extends Omit<CompiledFormula, "dependencie
 }

 export type Matrix<T = unknown> = T[][];
-export type ValueAndFormat = { value: CellValue; format?: Format };
+export type UnitData = { value: CellValue; format?: Format; message?: string };


UnitData doesn't really makes sense to me. But I don't have any brilliant better idea
CellEvaluation ? CellEvaluationResult ? CellFunctionResult ? EvaluationResult ? FunctionResult ? FormulaResult ? FormulaValue ? FormulaPayload (FP for short) ? FormulaData ? FData ?

FunctionResult

LucasLefevre · 2023-12-15T12:21:21Z

src/plugins/ui_core_views/cell_evaluation/evaluator.ts

@@ -210,17 +216,21 @@ export class Evaluator {
    }

    const cellId = cell.id;
-
+    const localeFormat = { format: cell.format, locale: this.getters.getLocale() };
    try {


A next step would be to change every throw in this file to returns.

src/plugins/ui_core_views/cell_evaluation/evaluator.ts

LucasLefevre · 2023-12-15T12:27:35Z

src/plugins/ui_core_views/evaluation_data_validation.ts

@@ -228,9 +226,11 @@ export class EvaluationDataValidationPlugin extends UIPlugin {
        );

        const evaluated = this.getters.evaluateFormula(sheetId, translatedFormula);
-        return evaluated && !isMatrix(evaluated) ? evaluated.toString() : "";
+        return evaluated?.toString() || "";


what if it's a matrix? Is it safe to remove the check?

I don't know what we want exactly:

Here I removed the check because the try catch below takes care of this case.
The evaluateFormula getter has been modified to throw when an array is received.
Perhaps we should first ask ourselves what exactly we want to do with this getters when it returns an array ?

Ok understood. After live discussion: this.getters.evaluateFormula shouldn't throw

src/types/cells.ts

src/plugins/ui_core_views/cell_evaluation/compilation_parameters.ts

anhe-odoo · 2024-01-09T12:15:13Z

src/formulas/compiler.ts

@@ -64,10 +64,10 @@ export function compileTokens(tokens: Token[]): CompiledFormula {
    const scope = new Scope();

    if (ast.type === "BIN_OPERATION" && ast.value === ":") {
-      throw new BadExpressionError(_t("Invalid formula"));
+      throw { value: CellErrorType.BadExpression, message: _t("Invalid formula") };


Personnally I prefere the old way to throw/return error. Why did you change this ?. Maybe we can keep something like a function BadExpressionError(message: string) so that we can have return BadExpressionError(_t("Invalid formula"));. Same for the other type of error ?

the goal would be that the formulas end up returning errors in the form of the normal payload rather than throwing errors, (performance reason)

I must admit I agree with Anthony. It feels very here where we are not in the context of a function implementation that would return a value (or an error).

BadExpressionError(message) and other errors could indeed be functions that result in {value: "#SomeError", message}

or keep BadExpressionError as a class extending Error but with an additional property value.
That way it would:

still be an error (and it feels natural to throw it)

respect the same interface { value, message }

Basically the same as now, but errorType becomes value

LucasLefevre · 2024-01-16T12:53:06Z

robodoo rebase-merge r+

robodoo · 2024-01-16T12:53:16Z

Branch saas-17.1 is not within my remit, imma just ignore it.

LucasLefevre · 2024-01-16T12:59:15Z

et maintenant ?
robodoo rebase-merge r+

robodoo · 2024-01-16T13:01:12Z

Pull request status dashboard.

robodoo · 2024-01-16T13:01:14Z

Merge method set to rebase and merge, using the PR as merge commit message.

robodoo · 2024-01-16T13:01:16Z

Merge method set to rebase and merge, using the PR as merge commit message.

robodoo · 2024-01-16T13:01:18Z

I'm sorry, @LucasLefevre: this PR is already reviewed, reviewing it again is useless.

robodoo · 2024-01-16T13:01:19Z

@laa-odoo @LucasLefevre I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.

we would like in a future commit change spreadsheet error object by string errors and keep the error message at the level of the formula payload, if we want to do this, we will encounter a problem with the 'compute' functions of the formulas because the compute functions only manage the value (and not the entire payload), so we will not be able to transcribe the errors with their message. To resolve this, this commit unifies all the computes ('compute', 'computeFormat', computeValueAndFormat') into a same 'compute' which manages the entire payload. To avoid too many changes, it is now the casting helpers (toNumber, toString...) which take the payload and extract the value. Task: 2969401

This commit changes the form of user errors in the formula evaluation process. With the previous commits, having a user error is no longer an event that destroys the process of evaluating a formula: A cell with a user error is a cell that can functionally have an interest in changing the result of a formula in one direction rather than another. Thus, it is possible for each sub-formula of the same formula to return a payload corresponding to an error. Before this commit, an error payload corresponds to an error object stored on the "value" attribute of the formula payload {value: errorObject, format?: string} Now, the important error information is stored at the payload level. {value: string, format?: string, message?: string} There are two advantages to doing this: - In terms of performance, it can be expensive to create an error object and throw it. This commit gives the possibility for future commits to no longer throw an error object but to simply return an error payload. - the form of the formula payload is close to evaluatedCelle --> code simplification Task: 2969401

Jest doesn't fail if a console.error or console.warn is triggered. With this commit, any such message will make the test to fail Task: 2969401

LucasLefevre · 2024-01-16T13:06:09Z

robodoo r+

- refactor the code used to create the evaluated cells - removes certain parameters not used when handling errors during evaluation Task: 2969401 Part-of: #2932

The purpose of this commit is to allow the formulas to return errors as results. Functionally we want formulas like TRANSPOSE to be able to transpose an error without invalidating the entire evaluation process. We also want formulas like LOOKUP not to be interrupted when the search range has one or more errors. To make this change we accepted errors in input and output on the formulas, It nevertheless remains possible to throw errors in the formulas: it is no longer the evaluation process at the cell level which is stopped but the evaluation process at the formula level. Technically there is no longer a "try catch" at the evaluator level, here each formula is encapsulated in a “try catch” when it enters the formula register Other point, the _lastFnCalled present in the evaluation context and allowing to know which is the last formula to have been called before the raise of an error is no longer used. The information is available during the encapsulation of the formula in the "try catch". So, _lastFnCalled has therefore been removed from the evaluation context. Task: 2969401 Part-of: #2932 Co-authored-by: Lucas Lefèvre (lul) <lul@odoo.com>

Before the previous commit, the lazy type arguments made it possible to not stop the entire evaluation process when an error was provided as an argument. Here, with the previous commit, the evaluation process is no longer stopped when an error is passed as formula input, So we no longer need lazy type arguments, This commit remove it. Task: 2969401 Part-of: #2932

Allow specific function to accept/throw errors when they are present in arguments Task: 2969401 Part-of: #2932

with the idea of extending ValueAndFormat to something more complex that can handle errors and their messages, we are renaming it to FPayload Task: 2969401 Part-of: #2932

we would like in a future commit change spreadsheet error object by string errors and keep the error message at the level of the formula payload, if we want to do this, we will encounter a problem with the 'compute' functions of the formulas because the compute functions only manage the value (and not the entire payload), so we will not be able to transcribe the errors with their message. To resolve this, this commit unifies all the computes ('compute', 'computeFormat', computeValueAndFormat') into a same 'compute' which manages the entire payload. To avoid too many changes, it is now the casting helpers (toNumber, toString...) which take the payload and extract the value. Task: 2969401 Part-of: #2932

This commit changes the form of user errors in the formula evaluation process. With the previous commits, having a user error is no longer an event that destroys the process of evaluating a formula: A cell with a user error is a cell that can functionally have an interest in changing the result of a formula in one direction rather than another. Thus, it is possible for each sub-formula of the same formula to return a payload corresponding to an error. Before this commit, an error payload corresponds to an error object stored on the "value" attribute of the formula payload {value: errorObject, format?: string} Now, the important error information is stored at the payload level. {value: string, format?: string, message?: string} There are two advantages to doing this: - In terms of performance, it can be expensive to create an error object and throw it. This commit gives the possibility for future commits to no longer throw an error object but to simply return an error payload. - the form of the formula payload is close to evaluatedCelle --> code simplification Task: 2969401 Part-of: #2932

Jest doesn't fail if a console.error or console.warn is triggered. With this commit, any such message will make the test to fail Task: 2969401 Part-of: #2932

With those commits, functions can handle errors more easily. See individual commits for details. The purpose of this commit is to allow the formulas to return errors as results. Functionally we want formulas like TRANSPOSE to be able to transpose an error without invalidating the entire evaluation process. We also want formulas like LOOKUP not to be interrupted when the search range has one or more errors. closes #2932 Task: 2969401 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>

With those commits, functions can handle errors more easily. See individual commits for details. The purpose of this commit is to allow the formulas to return errors as results. Functionally we want formulas like TRANSPOSE to be able to transpose an error without invalidating the entire evaluation process. We also want formulas like LOOKUP not to be interrupted when the search range has one or more errors. closes #3454 Task: 2969401 Forward-port-of: #2932 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 4 times, most recently from 350c0b7 to 818db7c Compare September 28, 2023 09:00

LucasLefevre reviewed Sep 28, 2023

View reviewed changes

laa-odoo force-pushed the master-formulas-handle-error2-laa branch from 818db7c to 0a65786 Compare September 28, 2023 14:21

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 4 times, most recently from a0b0713 to 8b2b53f Compare November 20, 2023 15:09

laa-odoo force-pushed the master-formulas-handle-error2-laa branch from 8b2b53f to c49698f Compare November 21, 2023 19:35

hokolomopo reviewed Dec 5, 2023

View reviewed changes

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 4 times, most recently from 61a747d to c42f2f6 Compare December 14, 2023 14:53

LucasLefevre reviewed Dec 15, 2023

View reviewed changes

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 7 times, most recently from 80bce50 to 6cb8d40 Compare December 21, 2023 11:26

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 2 times, most recently from bbae191 to 416e207 Compare January 8, 2024 13:31

laa-odoo force-pushed the master-formulas-handle-error2-laa branch 2 times, most recently from 5de6280 to b9c4ec4 Compare January 9, 2024 12:09

anhe-odoo reviewed Jan 9, 2024

View reviewed changes

LucasLefevre changed the title ~~Master formulas handle error2 laa~~ [IMP] evaluation: functions can handle errors Jan 16, 2024

LucasLefevre force-pushed the master-formulas-handle-error2-laa branch from f2eaf85 to 4b3e29c Compare January 16, 2024 12:53

laa-odoo and others added 3 commits January 16, 2024 14:04

[IMP] test: fail if console.error

da02f26

Jest doesn't fail if a console.error or console.warn is triggered. With this commit, any such message will make the test to fail Task: 2969401

LucasLefevre force-pushed the master-formulas-handle-error2-laa branch from 4b3e29c to da02f26 Compare January 16, 2024 13:05

robodoo pushed a commit that referenced this pull request Jan 16, 2024

[REF] cell evaluation: some cleaning

c4953a5

- refactor the code used to create the evaluated cells - removes certain parameters not used when handling errors during evaluation Task: 2969401 Part-of: #2932

robodoo pushed a commit that referenced this pull request Jan 16, 2024

[IMP] functions: accept/throw errors on specific function

0aa596f

Allow specific function to accept/throw errors when they are present in arguments Task: 2969401 Part-of: #2932

robodoo pushed a commit that referenced this pull request Jan 16, 2024

[REF] formulas: modify typing name

c967a6b

with the idea of extending ValueAndFormat to something more complex that can handle errors and their messages, we are renaming it to FPayload Task: 2969401 Part-of: #2932

robodoo pushed a commit that referenced this pull request Jan 16, 2024

[IMP] test: fail if console.error

55fb8ee

Jest doesn't fail if a console.error or console.warn is triggered. With this commit, any such message will make the test to fail Task: 2969401 Part-of: #2932

fw-bot mentioned this pull request Jan 16, 2024

[FW][IMP] evaluation: functions can handle errors #3454

Closed

robodoo temporarily deployed to merge January 16, 2024 13:18 Inactive

robodoo closed this Jan 16, 2024

fw-bot deleted the master-formulas-handle-error2-laa branch January 30, 2024 13:47

LucasLefevre mentioned this pull request Jul 9, 2024

[FIX] CF: Ignore errors thrown during CF evaluation #4597

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMP] evaluation: functions can handle errors #2932

[IMP] evaluation: functions can handle errors #2932

laa-odoo commented Sep 25, 2023 •

edited by LucasLefevre

Loading

robodoo commented Sep 25, 2023

hokolomopo left a comment

hokolomopo Dec 5, 2023

laa-odoo Dec 15, 2023

hokolomopo Dec 5, 2023

laa-odoo Dec 15, 2023

LucasLefevre left a comment

LucasLefevre Dec 15, 2023 •

edited

Loading

pro-odoo Dec 18, 2023

laa-odoo Dec 18, 2023

LucasLefevre Dec 15, 2023

LucasLefevre Dec 15, 2023

laa-odoo Dec 15, 2023

LucasLefevre Dec 18, 2023

anhe-odoo Jan 9, 2024 •

edited

Loading

laa-odoo Jan 9, 2024

LucasLefevre Jan 9, 2024

laa-odoo Jan 9, 2024 •

edited

Loading

LucasLefevre Jan 9, 2024 •

edited

Loading

LucasLefevre commented Jan 16, 2024

robodoo commented Jan 16, 2024

LucasLefevre commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

LucasLefevre commented Jan 16, 2024

[IMP] evaluation: functions can handle errors #2932

[IMP] evaluation: functions can handle errors #2932

Conversation

laa-odoo commented Sep 25, 2023 • edited by LucasLefevre Loading

robodoo commented Sep 25, 2023

hokolomopo 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

Choose a reason for hiding this comment

LucasLefevre left a comment

Choose a reason for hiding this comment

LucasLefevre Dec 15, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhe-odoo Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laa-odoo Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

LucasLefevre Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

LucasLefevre commented Jan 16, 2024

robodoo commented Jan 16, 2024

LucasLefevre commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

robodoo commented Jan 16, 2024

LucasLefevre commented Jan 16, 2024

laa-odoo commented Sep 25, 2023 •

edited by LucasLefevre

Loading

LucasLefevre Dec 15, 2023 •

edited

Loading

anhe-odoo Jan 9, 2024 •

edited

Loading

laa-odoo Jan 9, 2024 •

edited

Loading

LucasLefevre Jan 9, 2024 •

edited

Loading