[IMP] evaluation: implement curly brackets #7212
Conversation
|
|
laa-odoo
force-pushed
the
master-add-curly-brackets-laa
branch
6 times, most recently
from
bb47554 to
420ca7f
Compare
| @@ -332,20 +354,21 @@ export function astToFormula(ast: AST): string { | |||
| function leftOperandNeedsParenthesis(operationAST: ASTOperation | ASTUnaryOperation): boolean { | |||
| const mainOperator = operationAST.value; | |||
| const leftOperation = "left" in operationAST ? operationAST.left : operationAST.operand; | |||
| if (leftOperation.type !== "BIN_OPERATION") { | |||
There was a problem hiding this comment.
Why have you changed these lines ? It does the same thing (except the assignment of leftOperator), doesn't it ?
There was a problem hiding this comment.
These are some minor changes made when I re-dived into the code to better understand what it does. But in fact this is refactoring and does not belong here. I am removing it as well as the others you mentioned. ;)
There was a problem hiding this comment.
I didn't say anything; the change is necessary since I created a new AST (ASTArray) that no longer has the value attribute.
So, should we avoid this by keeping value to store the matrix and thus losing comprehension? It's up to the team to decide.
There was a problem hiding this comment.
Then shoudn't we also do the same change for mainOperator 2 lines above ?
There was a problem hiding this comment.
This is just so TypeScript can compile on
https://github.com/odoo/o-spreadsheet/pull/7212/files#diff-88a81a3427c2adbad1c2ca7ed98a682c8145fc3487db8eb01e60155b8d204043R361
mainOperator is defined as ASTOperation, so we can be sure that value exists on it.
| } | ||
| if (nextToken.type === "ARRAY_ROW_SEPARATOR") { | ||
| tokens.shift(); | ||
| if (expectValue) { |
There was a problem hiding this comment.
What if we want to have an empty cell inside the row of an array ? Isn't there any case where we sohuld want it ?
There was a problem hiding this comment.
Here, I've chosen to arbitrarily replicate Google's behavior. But we can ask ourselves why they return an error directly to the parser:
In their mindset, curly brackets are used to aggregate objects if and only if their dimensions are compatible. An empty value here would correspond to an object without dimensions, which would therefore not be compatible with the other objects.
--> So they allow themselves to return the error directly to the parser to avoid unnecessary evaluation.
(I suppose)
To the question, how to introduce empty cells: using the value 0, or a subsection composed of several 0, e.g.:
={ A1:B2, {0,0;0,0} }
| export function extractFormulaIdFromToken(tokenAtCursor: EnrichedToken) { | ||
| const idAst = tokenAtCursor.functionContext?.args[0]; | ||
| if (!idAst || !["STRING", "NUMBER"].includes(idAst.type)) { | ||
| if (!idAst || (idAst.type !== "STRING" && idAst.type !== "NUMBER")) { |
There was a problem hiding this comment.
Nitpick: Another useless diff. If you ally want to add them, maybe make another commit ?
There was a problem hiding this comment.
How can this be the same "maybe we don't have value" answer ? Here we have exactly the same behavior, don't we ?
There was a problem hiding this comment.
typescript fails to infer that with includes the type can only be "STRING" or "NUMBER". So it assumes it can also be "ARRAY" and the code fails to compile.
anhe-odoo
changed the title
laa-odoo
force-pushed
the
master-add-curly-brackets-laa
branch
from
420ca7f to
eee6b87
Compare
Allow entering literal arrays when composing formulas using curly brackets {}.
Task: 4735250
laa-odoo
force-pushed
the
master-add-curly-brackets-laa
branch
from
eee6b87 to
407e955
Compare
3 participants
Task: 4735250