Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2096 (2023-07-04 12:32:05)

refactor: Add TaskExpression to encapsulate Expression functions
<!--- Provide a general summary of your changes in the Title above -->

# Description

I found when first trying to implement `filter by function` that I was writing a bunch of code repetitively in order to call the functions in `Expression.ts` on `Task` objects.

So I have created this class `TaskExpression` to hide all the details of the functions in `Expression.ts`.

## Motivation and Context

This is a step on the way to implementing `filter by function`.

## How has this been tested?

By writing new tests.

(It is not yet called by any code in `src/`, so no manual testing.

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

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/QueryComponentOrError.ts b/src/Query/QueryComponentOrError.ts
index 1b0b0807..c2f17d4e 100644
--- a/src/Query/QueryComponentOrError.ts
+++ b/src/Query/QueryComponentOrError.ts
@@ -11,7 +11,7 @@ export class QueryComponentOrError<QueryComponent> {
     private _queryComponent: QueryComponent | undefined;
     private _error: string | undefined;
 
-    private constructor(instruction: string) {
+    protected constructor(instruction: string) {
         this.instruction = instruction;
     }
 
diff --git a/src/Scripting/Expression.ts b/src/Scripting/Expression.ts
index 9fea9845..3ee33396 100644
--- a/src/Scripting/Expression.ts
+++ b/src/Scripting/Expression.ts
@@ -1,13 +1,17 @@
 import type { Task } from '../Task';
+import { QueryComponentOrError } from '../Query/QueryComponentOrError';
 import { errorMessageForException } from '../lib/ExceptionTools';
 
+export class FunctionOrError extends QueryComponentOrError<Function> {}
+
 /**
  *  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
+ * @param task - during parsing, this can be null. During evaluation, it must be a Task
  */
-export function constructArguments(task: Task) {
+export function constructArguments(task: Task | null) {
+    // TODO Move this to TaskExpression
     const paramsArgs: [string, any][] = [
         // TODO Later, pass in the Query too, for access to file properties
         ['task', task],
@@ -23,17 +27,17 @@ export function constructArguments(task: Task) {
  * @see evaluateExpression
  * @see evaluateExpressionOrCatch
  */
-export function parseExpression(paramsArgs: [string, any][], arg: string): Function | string {
+export function parseExpression(paramsArgs: [string, any][], arg: string): FunctionOrError {
     const params = paramsArgs.map(([p]) => p);
     try {
         const expression: '' | null | Function = arg && new Function(...params, `return ${arg}`);
         if (expression instanceof Function) {
-            return expression;
+            return FunctionOrError.fromObject(arg, expression);
         }
         // I have not managed to write a test that reaches here:
-        return 'Error parsing group function';
+        return FunctionOrError.fromError(arg, 'Error parsing group function');
     } catch (e) {
-        return errorMessageForException(`Failed parsing expression "${arg}"`, e);
+        return FunctionOrError.fromError(arg, errorMessageForException(`Failed parsing expression "${arg}"`, e));
     }
 }
 
@@ -81,11 +85,10 @@ export function evaluateExpressionOrCatch(expression: Function, paramsArgs: [str
 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;
+    const functionOrError = parseExpression(paramsArgs, arg);
+    if (functionOrError.error) {
+        return functionOrError.error;
     }
 
-    return evaluateExpressionOrCatch(expression, paramsArgs, arg);
+    return evaluateExpressionOrCatch(functionOrError.queryComponent!, paramsArgs, arg);
 }
diff --git a/src/Scripting/TaskExpression.ts b/src/Scripting/TaskExpression.ts
new file mode 100644
index 00000000..d9525e56
--- /dev/null
+++ b/src/Scripting/TaskExpression.ts
@@ -0,0 +1,57 @@
+import type { Task } from '../Task';
+import {
+    FunctionOrError,
+    constructArguments,
+    evaluateExpression,
+    evaluateExpressionOrCatch,
+    parseExpression,
+} from './Expression';
+
+/**
+ * Encapsulate an expression that can be calculated from a {@link Task} object
+ */
+export class TaskExpression {
+    public readonly line: string;
+    private readonly functionOrError: FunctionOrError;
+
+    public constructor(line: string) {
+        this.line = line;
+        this.functionOrError = parseExpression(constructArguments(null), line);
+    }
+
+    public isValid() {
+        return this.functionOrError.error === undefined;
+    }
+
+    public get parseError(): string | undefined {
+        return this.functionOrError.error;
+    }
+
+    /**
+     * Evaluate the expression on this task, or throw an exception if the calculation failed
+     * @param task
+     *
+     * @see evaluateOrCatch
+     */
+    public evaluate(task: Task) {
+        if (!this.isValid()) {
+            throw Error(
+                `Error: Cannot evaluate an expression which is not valid: "${this.line}" gave error: "${this.parseError}"`,
+            );
+        }
+        return evaluateExpression(this.functionOrError.queryComponent!, constructArguments(task));
+    }
+
+    /**
+     * Evaluate the expression on this task, or return error text if the calculation failed
+     * @param task
+     *
+     * @see evaluate
+     */
+    public evaluateOrCatch(task: Task) {
+        if (!this.isValid()) {
+            return `Error: Cannot evaluate an expression which is not valid: "${this.line}" gave error: "${this.parseError}"`;
+        }
+        return evaluateExpressionOrCatch(this.functionOrError.queryComponent!, constructArguments(task), this.line);
+    }
+}


Test patch:
diff --git a/tests/Scripting/Expression.test.ts b/tests/Scripting/Expression.test.ts
index 48e205bc..3fbbb0dc 100644
--- a/tests/Scripting/Expression.test.ts
+++ b/tests/Scripting/Expression.test.ts
@@ -21,20 +21,20 @@ describe('Expression', () => {
 
     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(
+            // evaluateExpression() currently adds a return statement, to save user typing it.
+            expect(parseExpression([], 'return 42').error).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(
+            expect(parseExpression([], 'x(').error).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(
+            expect(parseExpression([], 'x())').error).toEqual(
                 'Error: Failed parsing expression "x())". The error message was: "SyntaxError: Unexpected token \')\'"',
             );
         });
@@ -45,9 +45,9 @@ describe('Expression', () => {
         const paramsArgs = constructArguments(task);
         const expression = parseExpression(paramsArgs, line);
         it('evaluateExpressionAndCatch() should report meaningful error message for invalid variable', () => {
-            expect(expression).not.toBeUndefined();
+            expect(expression.error).toBeUndefined();
 
-            const result = evaluateExpressionOrCatch(expression as Function, paramsArgs, line);
+            const result = evaluateExpressionOrCatch(expression.queryComponent!, paramsArgs, line);
             expect(result).toEqual(
                 'Error: Failed calculating expression "nonExistentVariable". The error message was: "ReferenceError: nonExistentVariable is not defined"',
             );
@@ -55,7 +55,7 @@ describe('Expression', () => {
 
         it('evaluateExpression() should throw exception for invalid variable', () => {
             const t = () => {
-                evaluateExpression(expression as Function, paramsArgs);
+                evaluateExpression(expression.queryComponent!, paramsArgs);
             };
             expect(t).toThrow(ReferenceError);
             expect(t).toThrowError('nonExistentVariable is not defined');
diff --git a/tests/Scripting/TaskExpression.test.ts b/tests/Scripting/TaskExpression.test.ts
new file mode 100644
index 00000000..6fe62fd4
--- /dev/null
+++ b/tests/Scripting/TaskExpression.test.ts
@@ -0,0 +1,86 @@
+import { TaskExpression } from '../../src/Scripting/TaskExpression';
+import { TaskBuilder } from '../TestingTools/TaskBuilder';
+
+describe('TaskExpression', () => {
+    describe('parsing', () => {
+        it('should report that a parsable line is valid', () => {
+            const line = 'task.description';
+
+            // Act
+            const taskExpression = new TaskExpression(line);
+
+            // Assert
+            expect(taskExpression.isValid()).toEqual(true);
+            expect(taskExpression.line).toEqual(line);
+            expect(taskExpression.parseError).toBeUndefined();
+        });
+
+        it('should report that a line with mismatched parentheses is invalid', () => {
+            const line = 'task.due.formatAsDate())';
+
+            // Act
+            const taskExpression = new TaskExpression(line);
+
+            // Assert
+            expect(taskExpression.isValid()).toEqual(false);
+            expect(taskExpression.line).toEqual(line);
+            expect(taskExpression.parseError).toEqual(
+                'Error: Failed parsing expression "task.due.formatAsDate())". The error message was: "SyntaxError: Unexpected token \')\'"',
+            );
+        });
+    });
+
+    describe('evaluating', () => {
+        it('should evaluate a valid line and give correct result', () => {
+            // Arrange
+            const taskExpression = new TaskExpression('task.description');
+
+            // Act
+            const result = taskExpression.evaluateOrCatch(new TaskBuilder().description('hello').build());
+
+            // Assert
+            expect(result).toEqual('hello');
+        });
+
+        it('should return error string as output if evaluating an expression that parsed OK fails at execution', () => {
+            // Arrange
+            const taskExpression = new TaskExpression('wibble');
+
+            // Act
+            const result = taskExpression.evaluateOrCatch(new TaskBuilder().build());
+
+            // Assert
+            expect(result).toEqual(
+                'Error: Failed calculating expression "wibble". The error message was: "ReferenceError: wibble is not defined"',
+            );
+
+            // Try again, using evaluate()
+            const t = () => {
+                taskExpression.evaluate(new TaskBuilder().build());
+            };
+            expect(t).toThrow(ReferenceError);
+            expect(t).toThrowError('wibble is not defined');
+        });
+
+        it('should give a meaningful error if evaluating a line that failed to parse', () => {
+            // Arrange
+            const taskExpression = new TaskExpression('task.due.formatAsDate(');
+            expect(taskExpression.isValid()).toEqual(false);
+
+            // Act
+            const result = taskExpression.evaluateOrCatch(new TaskBuilder().build());
+
+            // Assert
+            const expectedErrorMessage =
+                'Error: Cannot evaluate an expression which is not valid: "task.due.formatAsDate(" gave error: "Error: Failed parsing expression "task.due.formatAsDate(". The error message was: "SyntaxError: Unexpected token \'}\'""';
+            expect(result).toEqual(expectedErrorMessage);
+
+            // Try again, using evaluate()
+            const t = () => {
+                taskExpression.evaluate(new TaskBuilder().build());
+            };
+            expect(t).toThrow(Error);
+            expect(t).toThrowError(expectedErrorMessage);
+        });
+    });
+});