Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2873 (2024-06-03 09:25:53)

refactor: encapsulate adding children in `ListItem` constructor
# Types of changes

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)

## Description

Done by pairing with @claremacrae 

- Remove `children` parameter from `ListItem.constructor`
- Assign `children` inside the constructor
- Add tests and jsdocs showing the creation of `Task` & `ListItem` parent-child hierarchies

## Motivation and Context

- Improve usage of parent-child relationship code

## How has this been tested?

- unit tests

## Checklist

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

- [x] My contribution follows 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/Obsidian/Cache.ts b/src/Obsidian/Cache.ts
index 32abc4f2..aaf4bd39 100644
--- a/src/Obsidian/Cache.ts
+++ b/src/Obsidian/Cache.ts
@@ -102,8 +102,6 @@ export function getTasksFromFileContent2(
                             ...task,
                             parent: parentTask,
                         });
-
-                        parentTask.children.push(task);
                     }
 
                     line2Task.set(lineNumber, task);
diff --git a/src/Task/ListItem.ts b/src/Task/ListItem.ts
index 95b64978..1eb070ed 100644
--- a/src/Task/ListItem.ts
+++ b/src/Task/ListItem.ts
@@ -6,11 +6,14 @@ export class ListItem {
     public readonly originalMarkdown: string;
 
     public readonly parent: ListItem | null = null;
-    public readonly children: ListItem[];
+    public readonly children: ListItem[] = [];
 
-    constructor(originalMarkdown: string, parent: ListItem | null, children: ListItem[]) {
+    constructor(originalMarkdown: string, parent: ListItem | null) {
         this.originalMarkdown = originalMarkdown;
         this.parent = parent;
-        this.children = children;
+
+        if (parent !== null) {
+            parent.children.push(this);
+        }
     }
 }
diff --git a/src/Task/Task.ts b/src/Task/Task.ts
index 1e0a5024..adc6228b 100644
--- a/src/Task/Task.ts
+++ b/src/Task/Task.ts
@@ -116,7 +116,7 @@ export class Task extends ListItem {
         scheduledDateIsInferred: boolean;
         parent?: ListItem | null;
     }) {
-        super(originalMarkdown, parent, []);
+        super(originalMarkdown, parent);
         // NEW_TASK_FIELD_EDIT_REQUIRED
         this.status = status;
         this.description = description;
@@ -149,6 +149,11 @@ export class Task extends ListItem {
      * Takes the given line from an Obsidian note and returns a Task object.
      * Will check if Global Filter is present in the line.
      *
+     * If you want to specify a parent ListItem or Task after a fromLine call,
+     * you have to do the following:
+     * @example
+     *  const finalTask = new Task({ ...firstReadTask!, parent: parentListItem });
+     *
      * @static
      * @param {string} line - The full line in the note to parse.
      * @param {TaskLocation} taskLocation - The location of the task line
diff --git a/src/ui/EditTaskHelpers.ts b/src/ui/EditTaskHelpers.ts
index ffdd77fe..71b47d03 100644
--- a/src/ui/EditTaskHelpers.ts
+++ b/src/ui/EditTaskHelpers.ts
@@ -1,4 +1,6 @@
 import { capitalizeFirstLetter } from '../lib/StringHelpers';
+import { Recurrence } from '../Task/Recurrence';
+import type { EditableTask } from './EditableTask';
 
 /**
  * Returns contents for a `<label>` HTML element.
@@ -36,3 +38,28 @@ export function labelContentWithAccessKey(labelText: string, accessKey: string |
     labelContent = capitalizeFirstLetter(labelContent);
     return labelContent;
 }
+
+export function parseAndValidateRecurrence(editableTask: EditableTask) {
+    // NEW_TASK_FIELD_EDIT_REQUIRED
+    if (!editableTask.recurrenceRule) {
+        return { parsedRecurrence: '<i>not recurring</>', isRecurrenceValid: true };
+    }
+
+    const recurrenceFromText = Recurrence.fromText({
+        recurrenceRuleText: editableTask.recurrenceRule,
+        // Only for representation in the modal, no dates required.
+        startDate: null,
+        scheduledDate: null,
+        dueDate: null,
+    })?.toText();
+
+    if (!recurrenceFromText) {
+        return { parsedRecurrence: '<i>invalid recurrence rule</i>', isRecurrenceValid: false };
+    }
+
+    if (editableTask.startDate || editableTask.scheduledDate || editableTask.dueDate) {
+        return { parsedRecurrence: recurrenceFromText, isRecurrenceValid: true };
+    }
+
+    return { parsedRecurrence: '<i>due, scheduled or start date required</i>', isRecurrenceValid: false };
+}
diff --git a/src/ui/EditableTask.ts b/src/ui/EditableTask.ts
index 217bb502..8a96f95d 100644
--- a/src/ui/EditableTask.ts
+++ b/src/ui/EditableTask.ts
@@ -240,31 +240,6 @@ export class EditableTask {
         const today = doneDate ? doneDate : window.moment();
         return updatedTask.handleNewStatusWithRecurrenceInUsersOrder(this.status, today);
     }
-
-    public parseAndValidateRecurrence() {
-        // NEW_TASK_FIELD_EDIT_REQUIRED
-        if (!this.recurrenceRule) {
-            return { parsedRecurrence: '<i>not recurring</>', isRecurrenceValid: true };
-        }
-
-        const recurrenceFromText = Recurrence.fromText({
-            recurrenceRuleText: this.recurrenceRule,
-            // Only for representation in the modal, no dates required.
-            startDate: null,
-            scheduledDate: null,
-            dueDate: null,
-        })?.toText();
-
-        if (!recurrenceFromText) {
-            return { parsedRecurrence: '<i>invalid recurrence rule</i>', isRecurrenceValid: false };
-        }
-
-        if (this.startDate || this.scheduledDate || this.dueDate) {
-            return { parsedRecurrence: recurrenceFromText, isRecurrenceValid: true };
-        }
-
-        return { parsedRecurrence: '<i>due, scheduled or start date required</i>', isRecurrenceValid: false };
-    }
 }
 
 async function serialiseTaskId(task: Task, allTasks: Task[]) {
diff --git a/src/ui/RecurrenceEditor.svelte b/src/ui/RecurrenceEditor.svelte
index e3265552..e79376fd 100644
--- a/src/ui/RecurrenceEditor.svelte
+++ b/src/ui/RecurrenceEditor.svelte
@@ -1,7 +1,7 @@
 <script lang="ts">
     import { TASK_FORMATS } from '../Config/Settings';
     import type { EditableTask } from './EditableTask';
-    import { labelContentWithAccessKey } from './EditTaskHelpers';
+    import { labelContentWithAccessKey, parseAndValidateRecurrence } from './EditTaskHelpers';
 
     export let editableTask: EditableTask;
     export let isRecurrenceValid: boolean;
@@ -9,7 +9,7 @@
 
     let parsedRecurrence: string;
 
-    $: ({ parsedRecurrence, isRecurrenceValid } = editableTask.parseAndValidateRecurrence());
+    $: ({ parsedRecurrence, isRecurrenceValid } = parseAndValidateRecurrence(editableTask));
 
     const { recurrenceSymbol } = TASK_FORMATS.tasksPluginEmoji.taskSerializer.symbols;
 </script>


Test patch:
diff --git a/tests/Task/ListItem.test.ts b/tests/Task/ListItem.test.ts
index e35906b9..574df3c5 100644
--- a/tests/Task/ListItem.test.ts
+++ b/tests/Task/ListItem.test.ts
@@ -1,25 +1,64 @@
+/**
+ * @jest-environment jsdom
+ */
+import moment from 'moment/moment';
+import { TasksFile } from '../../src/Scripting/TasksFile';
+import { Task } from '../../src/Task/Task';
+import { TaskLocation } from '../../src/Task/TaskLocation';
 import { ListItem } from '../../src/Task/ListItem';
 
+window.moment = moment;
+
 describe('list item tests', () => {
     it('should create list item with empty children and absent parent', () => {
-        const listItem = new ListItem('', null, []);
+        const listItem = new ListItem('', null);
         expect(listItem).toBeDefined();
         expect(listItem.children).toEqual([]);
         expect(listItem.parent).toEqual(null);
     });
 
-    it('should create list item with a child', () => {
-        const childItem1 = new ListItem('', null, []);
-        const childItem2 = new ListItem('', null, []);
-        const listItem = new ListItem('', null, [childItem1, childItem2]);
+    it('should create a list item with 2 children', () => {
+        const listItem = new ListItem('', null);
+        const childItem1 = new ListItem('', listItem);
+        const childItem2 = new ListItem('', listItem);
         expect(listItem).toBeDefined();
+        expect(childItem1.parent).toEqual(listItem);
+        expect(childItem2.parent).toEqual(listItem);
         expect(listItem.children).toEqual([childItem1, childItem2]);
     });
 
-    it('should create list item with a parent', () => {
-        const parentItem = new ListItem('', null, []);
-        const listItem = new ListItem('', parentItem, []);
+    it('should create a list item with a parent', () => {
+        const parentItem = new ListItem('', null);
+        const listItem = new ListItem('', parentItem);
         expect(listItem).toBeDefined();
         expect(listItem.parent).toEqual(parentItem);
+        expect(parentItem.children).toEqual([listItem]);
+    });
+
+    it('should create a task child for a list item parent', () => {
+        const parentListItem = new ListItem('- parent item', null);
+        const firstReadTask = Task.fromLine({
+            line: '    - [ ] child task',
+            taskLocation: TaskLocation.fromUnknownPosition(new TasksFile('x.md')),
+            fallbackDate: null,
+        });
+
+        // ListItem.parent is immutable, so we have to create a new task
+        const finalTask = new Task({ ...firstReadTask!, parent: parentListItem });
+
+        expect(parentListItem.children).toEqual([finalTask]);
+        expect(finalTask.parent).toBe(parentListItem);
+    });
+
+    it('should create a list item child for a task parent', () => {
+        const parentTask = Task.fromLine({
+            line: '- [ ] parent task',
+            taskLocation: TaskLocation.fromUnknownPosition(new TasksFile('x.md')),
+            fallbackDate: null,
+        });
+        const childListItem = new ListItem('    - child item', parentTask);
+
+        expect(parentTask!.children).toEqual([childListItem]);
+        expect(childListItem.parent).toBe(parentTask);
     });
 });
diff --git a/tests/ui/EditTaskHelpers.test.ts b/tests/ui/EditTaskHelpers.test.ts
index 51a714be..fa43e1a5 100644
--- a/tests/ui/EditTaskHelpers.test.ts
+++ b/tests/ui/EditTaskHelpers.test.ts
@@ -1,4 +1,13 @@
-import { labelContentWithAccessKey } from '../../src/ui/EditTaskHelpers';
+/**
+ * @jest-environment jsdom
+ */
+
+import moment from 'moment/moment';
+import { EditableTask } from '../../src/ui/EditableTask';
+import { labelContentWithAccessKey, parseAndValidateRecurrence } from '../../src/ui/EditTaskHelpers';
+import { TaskBuilder } from '../TestingTools/TaskBuilder';
+
+window.moment = moment;
 
 describe('labelContentWithAccessKey() tests', () => {
     it.each([
@@ -38,3 +47,47 @@ describe('labelContentWithAccessKey() tests', () => {
         expect(labelContentWithAccessKey(labelText, accessKey)).toEqual(labelContent);
     });
 });
+
+describe('parseAndValidateRecurrence() tests', () => {
+    const emptyTask = new TaskBuilder().description('').build();
+    const editableTask = EditableTask.fromTask(emptyTask, [emptyTask]);
+
+    const noRecurrenceRule = (editableTask: EditableTask) => {
+        editableTask.recurrenceRule = '';
+        return editableTask;
+    };
+    const invalidRecurrenceRule = (editableTask: EditableTask) => {
+        editableTask.recurrenceRule = 'thisIsWrong';
+        return editableTask;
+    };
+    const withRecurrenceRuleButNoHappensDate = (editableTask: EditableTask) => {
+        editableTask.recurrenceRule = 'every day';
+        return editableTask;
+    };
+    const withRecurrenceRuleAndHappensDate = (editableTask: EditableTask) => {
+        editableTask.recurrenceRule = 'every 1 months when done'; // confirm that recurrence text is standardised
+        editableTask.startDate = '2024-05-20';
+        return editableTask;
+    };
+
+    it.each([
+        // editable task, expected parsed recurrence, expected recurrence validity
+        [noRecurrenceRule, '<i>not recurring</>', true],
+        [invalidRecurrenceRule, '<i>invalid recurrence rule</i>', false],
+        [withRecurrenceRuleButNoHappensDate, '<i>due, scheduled or start date required</i>', false],
+        [withRecurrenceRuleAndHappensDate, 'every month when done', true],
+    ])(
+        "editable task with '%s' fields should have '%s' parsed recurrence and its validity is %s",
+        (
+            taskEditor: (editableTask: EditableTask) => EditableTask,
+            expectedParsedRecurrence: string,
+            expectedRecurrenceValidity: boolean,
+        ) => {
+            const editedTask = taskEditor(editableTask);
+
+            const { parsedRecurrence, isRecurrenceValid } = parseAndValidateRecurrence(editedTask);
+            expect(parsedRecurrence).toEqual(expectedParsedRecurrence);
+            expect(isRecurrenceValid).toEqual(expectedRecurrenceValidity);
+        },
+    );
+});
diff --git a/tests/ui/EditableTask.test.ts b/tests/ui/EditableTask.test.ts
index dd67dfda..884dfaa9 100644
--- a/tests/ui/EditableTask.test.ts
+++ b/tests/ui/EditableTask.test.ts
@@ -239,47 +239,3 @@ describe('EditableTask tests', () => {
         expect(tasksClosestDay[0].dueDate).toEqualMoment(tuesdayAfter);
     });
 });
-
-describe('parseAndValidateRecurrence() tests', () => {
-    const emptyTask = new TaskBuilder().description('').build();
-
-    const noRecurrenceRule = (editableTask: EditableTask) => {
-        editableTask.recurrenceRule = '';
-        return editableTask;
-    };
-    const invalidRecurrenceRule = (editableTask: EditableTask) => {
-        editableTask.recurrenceRule = 'thisIsWrong';
-        return editableTask;
-    };
-    const withRecurrenceRuleButNoHappensDate = (editableTask: EditableTask) => {
-        editableTask.recurrenceRule = 'every day';
-        return editableTask;
-    };
-    const withRecurrenceRuleAndHappensDate = (editableTask: EditableTask) => {
-        editableTask.recurrenceRule = 'every 1 months when done'; // confirm that recurrence text is standardised
-        editableTask.startDate = '2024-05-20';
-        return editableTask;
-    };
-
-    it.each([
-        // editable task, expected parsed recurrence, expected recurrence validity
-        [noRecurrenceRule, '<i>not recurring</>', true],
-        [invalidRecurrenceRule, '<i>invalid recurrence rule</i>', false],
-        [withRecurrenceRuleButNoHappensDate, '<i>due, scheduled or start date required</i>', false],
-        [withRecurrenceRuleAndHappensDate, 'every month when done', true],
-    ])(
-        "editable task with '%s' fields should have '%s' parsed recurrence and its validity is %s",
-        (
-            taskEditor: (editableTask: EditableTask) => EditableTask,
-            expectedParsedRecurrence: string,
-            expectedRecurrenceValidity: boolean,
-        ) => {
-            const editableTask = EditableTask.fromTask(emptyTask, [emptyTask]);
-            const editedTask = taskEditor(editableTask);
-
-            const { parsedRecurrence, isRecurrenceValid } = editedTask.parseAndValidateRecurrence();
-            expect(parsedRecurrence).toEqual(expectedParsedRecurrence);
-            expect(isRecurrenceValid).toEqual(expectedRecurrenceValidity);
-        },
-    );
-});