Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2095 (2023-07-04 11:49:39)

refactor: Divide up evaluateExpression() for re-use
<!--- Provide a general summary of your changes in the Title above -->

# Description

A series of refactorings to divide up the original `evaluateExpression()` function in to separate parsing and evaluating functions.

## Motivation and Context

This is a step on the way to implementing `filter by function`.

## How has this been tested?

- Adding new tests
- Testing `group by function` manually in the demo vault.

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Changes visible to users:

Internal changes:

- [x] **Refactor** (prefix: `refactor` - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [ ] My change requires a change to the documentation.
- [ ] I have [updated the documentation](https://publish.obsidian.md/tasks-contributing/Documentation/About+Documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://publish.obsidian.md/tasks-contributing) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://publish.obsidian.md/tasks-contributing)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/Query/Filter/FunctionField.ts b/src/Query/Filter/FunctionField.ts
index da9ee618..61ecfa30 100644
--- a/src/Query/Filter/FunctionField.ts
+++ b/src/Query/Filter/FunctionField.ts
@@ -1,4 +1,4 @@
-import { evaluateExpression } from '../../Scripting/Expression';
+import { parseAndEvaluateExpression } from '../../Scripting/Expression';
 import type { Task } from '../../Task';
 import type { GrouperFunction } from '../Grouper';
 import { Grouper } from '../Grouper';
@@ -8,7 +8,7 @@ import { FilterOrErrorMessage } from './FilterOrErrorMessage';
 /**
  * A {@link Field} implement that accepts a JavaSscript expression to group tasks together.
  *
- * See also {@link evaluateExpression}
+ * See also {@link parseAndEvaluateExpression}
  */
 export class FunctionField extends Field {
     createFilterOrErrorMessage(line: string): FilterOrErrorMessage {
@@ -67,7 +67,7 @@ function createGrouperFunctionFromLine(line: string): GrouperFunction {
 
 export function groupByFunction(task: Task, arg: GroupingArg): string[] {
     try {
-        const result = evaluateExpression(task, arg);
+        const result = parseAndEvaluateExpression(task, arg);
 
         if (Array.isArray(result)) {
             return result.map((h) => h.toString());
diff --git a/src/Scripting/Expression.ts b/src/Scripting/Expression.ts
index 50216ac8..9fea9845 100644
--- a/src/Scripting/Expression.ts
+++ b/src/Scripting/Expression.ts
@@ -2,35 +2,90 @@ import type { Task } from '../Task';
 import { errorMessageForException } from '../lib/ExceptionTools';
 
 /**
- * Evaluate an arbitrary JavaScript expression on a Task object
- * @param task - a {@link Task} object
- * @param arg - a string, such as `task.path.startsWith("journal/") ? "journal/" : task.path`
- *
- * Currently any errors are returned as string error messages, starting with the word 'Error'.
- *
- * @todo Implement a type-safe mechanism to report error messages distinct from expression results.
- *
- * See also {@link FunctionField} which exposes this facility to users.
+ *  From: https://www.educative.io/answers/parameter-vs-argument
+ *      A parameter is a variable in a function definition. It is a placeholder and hence does not have a concrete value.
+ *      An argument is a value passed during function invocation.
+ * @param task
  */
-export function evaluateExpression(task: Task, arg: string | null) {
+export function constructArguments(task: Task) {
     const paramsArgs: [string, any][] = [
         // TODO Later, pass in the Query too, for access to file properties
         ['task', task],
     ];
+    return paramsArgs;
+}
 
+/**
+ * Parse a JavaScript expression, and return either a Function or an error message in a string.
+ * @param paramsArgs
+ * @param arg
+ *
+ * @see evaluateExpression
+ * @see evaluateExpressionOrCatch
+ */
+export function parseExpression(paramsArgs: [string, any][], arg: string): Function | string {
     const params = paramsArgs.map(([p]) => p);
-    const expression = arg && new Function(...params, `return ${arg}`);
-
-    if (!(expression instanceof Function)) {
+    try {
+        const expression: '' | null | Function = arg && new Function(...params, `return ${arg}`);
+        if (expression instanceof Function) {
+            return expression;
+        }
         // I have not managed to write a test that reaches here:
         return 'Error parsing group function';
+    } catch (e) {
+        return errorMessageForException(`Failed parsing expression "${arg}"`, e);
     }
+}
 
+/**
+ * Evaluate an arbitrary JavaScript expression, throwing an exception if the calculation failed.
+ * @param expression
+ * @param paramsArgs
+ *
+ * @see parseExpression
+ * @see evaluateExpressionOrCatch
+ */
+export function evaluateExpression(expression: Function, paramsArgs: [string, any][]) {
     const args = paramsArgs.map(([_, a]) => a);
+    return expression(...args);
+}
 
+/**
+ * Evaluate an arbitrary JavaScript expression, returning an error message if the calculation failed.
+ * @param expression
+ * @param paramsArgs
+ * @param arg
+ *
+ * @see parseExpression
+ * @see evaluateExpression
+ */
+export function evaluateExpressionOrCatch(expression: Function, paramsArgs: [string, any][], arg: string) {
     try {
-        return expression(...args);
+        return evaluateExpression(expression, paramsArgs);
     } catch (e) {
         return errorMessageForException(`Failed calculating expression "${arg}"`, e);
     }
 }
+
+/**
+ * Evaluate an arbitrary JavaScript expression on a Task object
+ * @param task - a {@link Task} object
+ * @param arg - a string, such as `task.path.startsWith("journal/") ? "journal/" : task.path`
+ *
+ * Currently any errors are returned as string error messages, starting with the word 'Error'.
+ *
+ * @todo Implement a type-safe mechanism to report error messages distinct from expression results.
+ *
+ * See also {@link FunctionField} which exposes this facility to users.
+ */
+export function parseAndEvaluateExpression(task: Task, arg: string) {
+    const paramsArgs = constructArguments(task);
+
+    const expression = parseExpression(paramsArgs, arg);
+    if (typeof expression === 'string') {
+        // It's an error message
+        return expression;
+    }
+
+    return evaluateExpressionOrCatch(expression, paramsArgs, arg);
+}


Test patch:
diff --git a/tests/Scripting/Expression.test.ts b/tests/Scripting/Expression.test.ts
index 3d9006af..48e205bc 100644
--- a/tests/Scripting/Expression.test.ts
+++ b/tests/Scripting/Expression.test.ts
@@ -3,7 +3,13 @@
  */
 import moment from 'moment';
 
-import { evaluateExpression } from '../../src/Scripting/Expression';
+import {
+    constructArguments,
+    evaluateExpression,
+    evaluateExpressionOrCatch,
+    parseAndEvaluateExpression,
+    parseExpression,
+} from '../../src/Scripting/Expression';
 import { TaskBuilder } from '../TestingTools/TaskBuilder';
 import { verifyMarkdownForDocs } from '../TestingTools/VerifyMarkdownTable';
 import { formatToRepresentType } from './ScriptingTestHelpers';
@@ -11,8 +17,56 @@ import { formatToRepresentType } from './ScriptingTestHelpers';
 window.moment = moment;
 
 describe('Expression', () => {
+    const task = TaskBuilder.createFullyPopulatedTask();
+
+    describe('detect errors at parse stage', () => {
+        it('should report meaningful error message for duplicate return statement', () => {
+            // parseExpression() currently adds a return statement, to save user typing it.
+            expect(parseExpression([], 'return 42')).toEqual(
+                'Error: Failed parsing expression "return 42". The error message was: "SyntaxError: Unexpected token \'return\'"',
+            );
+        });
+
+        it('should report meaningful error message for parentheses too few parentheses', () => {
+            expect(parseExpression([], 'x(')).toEqual(
+                'Error: Failed parsing expression "x(". The error message was: "SyntaxError: Unexpected token \'}\'"',
+            );
+        });
+
+        it('should report meaningful error message for parentheses too many parentheses', () => {
+            expect(parseExpression([], 'x())')).toEqual(
+                'Error: Failed parsing expression "x())". The error message was: "SyntaxError: Unexpected token \')\'"',
+            );
+        });
+    });
+
+    describe('detect errors at evaluation time', () => {
+        const line = 'nonExistentVariable';
+        const paramsArgs = constructArguments(task);
+        const expression = parseExpression(paramsArgs, line);
+        it('evaluateExpressionAndCatch() should report meaningful error message for invalid variable', () => {
+            expect(expression).not.toBeUndefined();
+
+            const result = evaluateExpressionOrCatch(expression as Function, paramsArgs, line);
+            expect(result).toEqual(
+                'Error: Failed calculating expression "nonExistentVariable". The error message was: "ReferenceError: nonExistentVariable is not defined"',
+            );
+        });
+
+        it('evaluateExpression() should throw exception for invalid variable', () => {
+            const t = () => {
+                evaluateExpression(expression as Function, paramsArgs);
+            };
+            expect(t).toThrow(ReferenceError);
+            expect(t).toThrowError('nonExistentVariable is not defined');
+        });
+
+        it('should report unknown for invalid task property', () => {
+            expect(parseAndEvaluateExpression(task, 'task.iAmNotAKnownTaskProperty')).toEqual(undefined);
+        });
+    });
+
     it('result', () => {
-        const task = TaskBuilder.createFullyPopulatedTask();
         const expressions = [
             "'hello'",
             '"hello"',
@@ -38,7 +92,7 @@ describe('Expression', () => {
 
         let markdown = '~~~text\n';
         for (const expression of expressions) {
-            const result = evaluateExpression(task, expression);
+            const result = parseAndEvaluateExpression(task, expression);
             markdown += `${expression} => ${formatToRepresentType(result)}\n`;
         }
         markdown += '~~~\n';
diff --git a/tests/Scripting/TaskProperties.test.ts b/tests/Scripting/TaskProperties.test.ts
index d0a92c9c..a25d3157 100644
--- a/tests/Scripting/TaskProperties.test.ts
+++ b/tests/Scripting/TaskProperties.test.ts
@@ -3,7 +3,7 @@
  */
 
 import moment from 'moment';
-import { evaluateExpression } from '../../src/Scripting/Expression';
+import { parseAndEvaluateExpression } from '../../src/Scripting/Expression';
 import { Status } from '../../src/Status';
 
 import { TaskBuilder } from '../TestingTools/TaskBuilder';
@@ -20,8 +20,8 @@ describe('task', () => {
         const task1 = TaskBuilder.createFullyPopulatedTask();
         const task2 = new TaskBuilder().description('minimal task').status(Status.makeInProgress()).build();
         for (const field of fields) {
-            const value1 = evaluateExpression(task1, field);
-            const value2 = evaluateExpression(task2, field);
+            const value1 = parseAndEvaluateExpression(task1, field);
+            const value2 = parseAndEvaluateExpression(task2, field);
             const cells = [
                 addBackticks(field),
                 addBackticks(determineExpressionType(value1)),