Display spreadsheet errors and disable math.js internal functions #3439
Conversation
dbraquart
left a comment
dbraquart
left a comment
There was a problem hiding this comment.
code : a few remarks
tests: ok
| return validation; | ||
| } | ||
| return result; | ||
| } catch (e) { |
There was a problem hiding this comment.
maybe we could manage any "evaluate" error from MathJS in this catch. Next step ?
There was a problem hiding this comment.
Precisely :) I elaborated on this in the going further section or the summary
|
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
There was a problem hiding this comment.
I think it can be simplified a lot. Think about it like the evaluation either returns a value or a ValidationError. In the end for each column definition, is it is a value we take DefaultCellRenderer and if it is a ValidationError we take ErrorCellRenderer.
| import { SortParams } from '../hooks/use-custom-aggrid-sort'; | ||
| import { COLUMN_TYPES, CustomCellType } from '../custom-aggrid-header.type'; | ||
| import type { UUID } from 'node:crypto'; | ||
| import { ValidationResult } from '../../spreadsheet-view/columns/utils/formula-validator'; |
| import { ICellRendererParams } from 'ag-grid-community'; | ||
| import { CustomCellRendererProps } from 'ag-grid-react'; | ||
| import { mergeSx, type MuiStyles } from '@gridsuite/commons-ui'; | ||
| import { IntlShape } from 'react-intl'; |
| } | ||
|
|
||
| export const formatValidationResult = (isValid: boolean, messageId?: string): ValidationResult => { | ||
| return { isValid: isValid, error: messageId }; |
There was a problem hiding this comment.
| return { isValid: isValid, error: messageId }; | |
| return { isValid, error: messageId }; |
| return isValidationResult(value) && !value.isValid; | ||
| } | ||
|
|
||
| export const formatValidationResult = (isValid: boolean, messageId?: string): ValidationResult => { |
There was a problem hiding this comment.
This is not really helpful
| ); | ||
|
|
||
| export const ErrorCellRenderer = (props: ErrorCellRendererParams) => { | ||
| const errorMessage = props.intl.formatMessage({ id: props.value?.error }); |
There was a problem hiding this comment.
You are in react context here already, you can use useIntl directly you don't need to pass it through props
| }; | ||
| }); | ||
|
|
||
| const addFormulaErrorsRenderer = (intl: IntlShape, columns: CustomColDef[]): CustomColDef[] => { |
There was a problem hiding this comment.
Why don't you add it in mapColumns directly ?
| import { IntlShape, useIntl } from 'react-intl'; | ||
| import { useMemo } from 'react'; | ||
|
|
||
| export const useColumnDefinitions = (tableDefinition: SpreadsheetTabDefinition) => { |
There was a problem hiding this comment.
I think this is not useful
| import { MAX_FORMULA_CHARACTERS } from '../../constants'; | ||
|
|
||
| interface ValidationResult { | ||
| export interface ValidationResult { |
There was a problem hiding this comment.
Maybe name it ValidationError directly with
{
error: string;
}
| }; | ||
|
|
||
| export const validateFormulaResult = (value: any, type: COLUMN_TYPES): ValidationResult => { | ||
| if (isValidationResult(value)) { |
There was a problem hiding this comment.
In this method it could be, if check passes returns the orginal value, if not returns a ValidationError
PR Summary
New Features
ErrorCellRenderercomponent to display#ERRORfor invalid formula results, with localized tooltip messages.math.tsto return structured validation errors instead of throwing exceptions.spreadsheet-en.tsandspreadsheet-fr.ts.Behavior Changes
column-mapper.ts:ErrorCellRendererwhen a formula result is invalid.isValidationResultto determine cell rendering behavior through AG gridcellRendererSelectorpropWhen both occur, forbidden math.js errors take priority (can be changed).
Other
import,parse,simplify, etc.) now return validation errors instead of exceptions.Going Further
This system can be incremented on to handle more math.js resolution errors by tweaking the
createValueGetterfunction.In particular, additional error thrown during formula evaluation can be caught in its
catchblock and returned as aValidationResult. This would allow theErrorCellRendererto display consistent error feedback for a wider range of math.js errors.Additionnaly the
ErrorCellRenderercould be specialized to display a diferent code depending on the type of caught error