Repository: obsidian-tasks-group/obsidian-tasks
Pull request #3224 (2024-12-06 12:26:10)

refactor: Introduce FileParser class
# 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

- Convert `parseFileContent()` in to a class `FileParser`.
-  By making some parameters fields, I can start extracting methods that have a reasonable number of parameters themselves.
- Initially, the big win is the extraction of `FileParser.parseFileContent()`, which greatly reduced the complexity of `FileParser.parseLine()`

Note that some of the new fields may turn out to be redundant - but I wanted to merge progress so far, why the diffs are still moderately readable.

Note that because of a change in indentation in some of this code, the diffs are easier to read in this Web UI if enabling GitHub's **'Hide whitespace'** option:

<img width="450" alt="image" src="https://github.com/user-attachments/assets/b1e35428-0256-4824-8116-e334856c87a2">


## Motivation and Context

Preparing to fix:

- https://github.com/obsidian-tasks-group/obsidian-tasks/issues/3170

## How has this been tested?

- Automated tests
- Exploratory testing

## 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 8ce5ddbf..d0380a57 100644
--- a/src/Obsidian/Cache.ts
+++ b/src/Obsidian/Cache.ts
@@ -10,7 +10,7 @@ import { getSettings } from '../Config/Settings';
 import { Lazy } from '../lib/Lazy';
 import { Logger, logging } from '../lib/logging';
 import type { TasksEvents } from './TasksEvents';
-import { parseFileContent } from './FileParser';
+import { FileParser } from './FileParser';
 
 export enum State {
     Cold = 'Cold',
@@ -307,7 +307,8 @@ export class Cache {
         errorReporter: (e: any, filePath: string, listItem: ListItemCache, line: string) => void,
         logger: Logger,
     ): Task[] {
-        return parseFileContent(filePath, fileContent, listItems, logger, fileCache, errorReporter);
+        const fileParser = new FileParser(filePath, fileContent, listItems, logger, fileCache, errorReporter);
+        return fileParser.parseFileContent();
     }
 
     private reportTaskParsingErrorToUser(e: any, filePath: string, listItem: ListItemCache, line: string) {
diff --git a/src/Obsidian/FileParser.ts b/src/Obsidian/FileParser.ts
index 9ab3e7f6..2e02d566 100644
--- a/src/Obsidian/FileParser.ts
+++ b/src/Obsidian/FileParser.ts
@@ -8,92 +8,129 @@ import { ListItem } from '../Task/ListItem';
 import { TaskLocation } from '../Task/TaskLocation';
 import { Cache } from './Cache';
 
-export function parseFileContent(
-    filePath: string,
-    fileContent: string,
-    listItems: ListItemCache[] | undefined,
-    logger: Logger,
-    fileCache: CachedMetadata,
-    errorReporter: (e: any, filePath: string, listItem: ListItemCache, line: string) => void,
-) {
-    const tasks: Task[] = [];
-    if (listItems === undefined) {
-        // When called via Cache, this function would never be called or files without list items.
-        // It is useful for tests to be act gracefully on sample Markdown files with no list items, however.
-        return tasks;
+export class FileParser {
+    private readonly filePath: string;
+    private readonly fileContent: string;
+    private readonly listItems: ListItemCache[];
+    private readonly logger: Logger;
+    private readonly fileCache: CachedMetadata;
+    private readonly errorReporter: (e: any, filePath: string, listItem: ListItemCache, line: string) => void;
+
+    private readonly fileLines: string[];
+    private readonly line2ListItem: Map<number, ListItem> = new Map();
+    private readonly tasks: Task[] = [];
+    private readonly dateFromFileName: Lazy<moment.Moment | null>;
+
+    constructor(
+        filePath: string,
+        fileContent: string,
+        listItems: ListItemCache[],
+        logger: Logger,
+        fileCache: CachedMetadata,
+        errorReporter: (e: any, filePath: string, listItem: ListItemCache, line: string) => void,
+    ) {
+        this.filePath = filePath;
+        this.fileContent = fileContent;
+        this.listItems = listItems;
+        this.logger = logger;
+        this.fileCache = fileCache;
+        this.errorReporter = errorReporter;
+        this.fileLines = this.fileContent.split('\n');
+
+        // Lazily store date extracted from filename to avoid parsing more than needed
+        this.dateFromFileName = new Lazy(() => DateFallback.fromPath(this.filePath));
     }
 
-    const tasksFile = new TasksFile(filePath, fileCache);
-    const fileLines = fileContent.split('\n');
-    const linesInFile = fileLines.length;
-
-    // Lazily store date extracted from filename to avoid parsing more than needed
-    // this.logger.debug(`getTasksFromFileContent() reading ${file.path}`);
-    const dateFromFileName = new Lazy(() => DateFallback.fromPath(filePath));
-
-    // We want to store section information with every task so
-    // that we can use that when we post process the markdown
-    // rendered lists.
-    let currentSection: SectionCache | null = null;
-    let sectionIndex = 0;
-    const line2ListItem: Map<number, ListItem> = new Map();
-    for (const listItem of listItems) {
-        const lineNumber = listItem.position.start.line;
-        if (lineNumber >= linesInFile) {
-            /*
-                Obsidian CachedMetadata has told us that there is a task on lineNumber, but there are
-                not that many lines in the file.
-
-                This was the underlying cause of all the 'Stuck on "Loading Tasks..."' messages,
-                as it resulted in the line 'undefined' being parsed.
-
-                Somehow the file had been shortened whilst Obsidian was closed, meaning that
-                when Obsidian started up, it got the new file content, but still had the old cached
-                data about locations of list items in the file.
-             */
-            logger.debug(
-                `${filePath} Obsidian gave us a line number ${lineNumber} past the end of the file. ${linesInFile}.`,
-            );
-            return tasks;
-        }
-        if (currentSection === null || currentSection.position.end.line < lineNumber) {
-            // We went past the current section (or this is the first task).
-            // Find the section that is relevant for this task and the following of the same section.
-            currentSection = Cache.getSection(lineNumber, fileCache.sections);
-            sectionIndex = 0;
+    /**
+     * **Warning**: This is designed to only be called **once per {@link FileParser} instance**.
+     */
+    public parseFileContent() {
+        if (this.listItems === undefined) {
+            // When called via Cache, this function would never be called or files without list items.
+            // It is useful for tests to be act gracefully on sample Markdown files with no list items, however.
+            return this.tasks;
         }
 
-        if (currentSection === null) {
-            // Cannot process a task without a section.
-            continue;
-        }
+        const tasksFile = new TasksFile(this.filePath, this.fileCache);
+        const linesInFile = this.fileLines.length;
+
+        // this.logger.debug(`FileParser.parseFileContent() reading ${this.filePath}`);
 
-        const line = fileLines[lineNumber];
-        if (line === undefined) {
-            logger.debug(`${filePath}: line ${lineNumber} - ignoring 'undefined' line.`);
-            continue;
+        // We want to store section information with every task so
+        // that we can use that when we post process the markdown
+        // rendered lists.
+        let currentSection: SectionCache | null = null;
+        let sectionIndex = 0;
+        for (const listItem of this.listItems) {
+            const lineNumber = listItem.position.start.line;
+            if (lineNumber >= linesInFile) {
+                /*
+                    Obsidian CachedMetadata has told us that there is a task on lineNumber, but there are
+                    not that many lines in the file.
+
+                    This was the underlying cause of all the 'Stuck on "Loading Tasks..."' messages,
+                    as it resulted in the line 'undefined' being parsed.
+
+                    Somehow the file had been shortened whilst Obsidian was closed, meaning that
+                    when Obsidian started up, it got the new file content, but still had the old cached
+                    data about locations of list items in the file.
+                 */
+                this.logger.debug(
+                    `${this.filePath} Obsidian gave us a line number ${lineNumber} past the end of the file. ${linesInFile}.`,
+                );
+                return this.tasks;
+            }
+            if (currentSection === null || currentSection.position.end.line < lineNumber) {
+                // We went past the current section (or this is the first task).
+                // Find the section that is relevant for this task and the following of the same section.
+                currentSection = Cache.getSection(lineNumber, this.fileCache.sections);
+                sectionIndex = 0;
+            }
+
+            if (currentSection === null) {
+                // Cannot process a task without a section.
+                continue;
+            }
+
+            const line = this.fileLines[lineNumber];
+            if (line === undefined) {
+                this.logger.debug(`${this.filePath}: line ${lineNumber} - ignoring 'undefined' line.`);
+                continue;
+            }
+
+            const taskLocation = new TaskLocation(
+                tasksFile,
+                lineNumber,
+                currentSection.position.start.line,
+                sectionIndex,
+                Cache.getPrecedingHeader(lineNumber, this.fileCache.headings),
+            );
+            sectionIndex = this.parseLine(listItem, line, taskLocation, lineNumber, sectionIndex);
         }
 
-        const taskLocation = new TaskLocation(
-            tasksFile,
-            lineNumber,
-            currentSection.position.start.line,
-            sectionIndex,
-            Cache.getPrecedingHeader(lineNumber, fileCache.headings),
-        );
+        return this.tasks;
+    }
+
+    private parseLine(
+        listItem: ListItemCache,
+        line: string,
+        taskLocation: TaskLocation,
+        lineNumber: number,
+        sectionIndex: number,
+    ) {
         if (listItem.task !== undefined) {
             let task;
             try {
                 task = Task.fromLine({
                     line,
                     taskLocation: taskLocation,
-                    fallbackDate: dateFromFileName.value,
+                    fallbackDate: this.dateFromFileName.value,
                 });
 
                 if (task !== null) {
                     // listItem.parent could be negative if the parent is not found (in other words, it is a root task).
                     // That is not a problem, as we never put a negative number in line2ListItem map, so parent will be null.
-                    const parentListItem: ListItem | null = line2ListItem.get(listItem.parent) ?? null;
+                    const parentListItem: ListItem | null = this.line2ListItem.get(listItem.parent) ?? null;
                     if (parentListItem !== null) {
                         task = new Task({
                             ...task,
@@ -101,25 +138,21 @@ export function parseFileContent(
                         });
                     }
 
-                    line2ListItem.set(lineNumber, task);
+                    this.line2ListItem.set(lineNumber, task);
                 }
             } catch (e) {
-                errorReporter(e, filePath, listItem, line);
-                continue;
+                this.errorReporter(e, this.filePath, listItem, line);
+                return sectionIndex;
             }
 
             if (task !== null) {
                 sectionIndex++;
-                tasks.push(task);
+                this.tasks.push(task);
             }
         } else {
-            const lineNumber = listItem.position.start.line;
-
-            const parentListItem: ListItem | null = line2ListItem.get(listItem.parent) ?? null;
-
-            line2ListItem.set(lineNumber, new ListItem(fileLines[lineNumber], parentListItem));
+            const parentListItem: ListItem | null = this.line2ListItem.get(listItem.parent) ?? null;
+            this.line2ListItem.set(lineNumber, new ListItem(this.fileLines[lineNumber], parentListItem));
         }
+        return sectionIndex;
     }
-
-    return tasks;
 }
diff --git a/src/Task/TaskLocation.ts b/src/Task/TaskLocation.ts
index 6e66388d..03ab4bfe 100644
--- a/src/Task/TaskLocation.ts
+++ b/src/Task/TaskLocation.ts
@@ -85,4 +85,9 @@ export class TaskLocation {
     public get hasKnownPath(): boolean {
         return this.path !== '';
     }
+
+    public allFieldsExceptTasksFileForTesting() {
+        const { _tasksFile, ...rest } = { ...this };
+        return rest;
+    }
 }


Test patch:
diff --git a/resources/sample_vaults/Tasks-Demo/Test Data/multiple_headings.md b/resources/sample_vaults/Tasks-Demo/Test Data/multiple_headings.md
index e1198aab..ad9e90ce 100644
--- a/resources/sample_vaults/Tasks-Demo/Test Data/multiple_headings.md
+++ b/resources/sample_vaults/Tasks-Demo/Test Data/multiple_headings.md
@@ -1,7 +1,17 @@
 # multiple_headings
 
+## Level 2 heading with no taks
+
 ## Level 2 heading
 
+- [ ] #task Task 1 in Level 2 heading - in 'multiple_headings' - list 1
+- [ ] #task task 2 in Level 2 heading - in 'multiple_headings' - list 1
+
+Divider to create a new section
+
+- [ ] #task task 3 in Level 2 heading - in 'multiple_headings' - list 2
+
 ### Level 3 heading
 
-- [ ] #task Task in 'multiple_headings'
+- [ ] #task Task 4 in 'multiple_headings' - in 'multiple_headings' - list 3
+- [ ] #task Task 5 in 'multiple_headings' - in 'multiple_headings' - list 3
diff --git a/tests/Obsidian/FileParser.test.ts b/tests/Obsidian/FileParser.test.ts
new file mode 100644
index 00000000..eeb707e6
--- /dev/null
+++ b/tests/Obsidian/FileParser.test.ts
@@ -0,0 +1,47 @@
+import { readTasksFromSimulatedFile } from './SimulatedFile';
+import multiple_headings from './__test_data__/multiple_headings.json';
+
+describe('FileParser', () => {
+    it('should set all non-TasksFile data in TaskLocation', () => {
+        // This test exists to detect accidental changes to the task-reading code.
+        // I found that intentionally breaking the setting of _sectionIndex was
+        // not caught by any other tests.
+
+        const tasks = readTasksFromSimulatedFile(multiple_headings);
+        const locationDataExceptTasksFile = tasks.map((task) => task.taskLocation.allFieldsExceptTasksFileForTesting());
+        expect(locationDataExceptTasksFile).toMatchInlineSnapshot(`
+            [
+              {
+                "_lineNumber": 6,
+                "_precedingHeader": "Level 2 heading",
+                "_sectionIndex": 0,
+                "_sectionStart": 6,
+              },
+              {
+                "_lineNumber": 7,
+                "_precedingHeader": "Level 2 heading",
+                "_sectionIndex": 1,
+                "_sectionStart": 6,
+              },
+              {
+                "_lineNumber": 11,
+                "_precedingHeader": "Level 2 heading",
+                "_sectionIndex": 0,
+                "_sectionStart": 11,
+              },
+              {
+                "_lineNumber": 15,
+                "_precedingHeader": "Level 3 heading",
+                "_sectionIndex": 0,
+                "_sectionStart": 15,
+              },
+              {
+                "_lineNumber": 16,
+                "_precedingHeader": "Level 3 heading",
+                "_sectionIndex": 1,
+                "_sectionStart": 15,
+              },
+            ]
+        `);
+    });
+});
diff --git a/tests/Obsidian/SimulatedFile.ts b/tests/Obsidian/SimulatedFile.ts
index efb2d2a6..d776b452 100644
--- a/tests/Obsidian/SimulatedFile.ts
+++ b/tests/Obsidian/SimulatedFile.ts
@@ -1,6 +1,6 @@
 import type { CachedMetadata } from 'obsidian';
 import { logging } from '../../src/lib/logging';
-import { parseFileContent } from '../../src/Obsidian/FileParser';
+import { FileParser } from '../../src/Obsidian/FileParser';
 import { setCurrentCacheFile } from '../__mocks__/obsidian';
 
 export interface SimulatedFile {
@@ -18,7 +18,7 @@ export interface SimulatedFile {
 export function readTasksFromSimulatedFile(testData: SimulatedFile) {
     const logger = logging.getLogger('testCache');
     setCurrentCacheFile(testData);
-    return parseFileContent(
+    const fileParser = new FileParser(
         testData.filePath,
         testData.fileContents,
         testData.cachedMetadata.listItems!,
@@ -26,6 +26,7 @@ export function readTasksFromSimulatedFile(testData: SimulatedFile) {
         testData.cachedMetadata,
         errorReporter,
     );
+    return fileParser.parseFileContent();
 }
 
 function errorReporter() {
diff --git a/tests/Obsidian/__test_data__/multiple_headings.json b/tests/Obsidian/__test_data__/multiple_headings.json
index ceb37f1b..251c4794 100644
--- a/tests/Obsidian/__test_data__/multiple_headings.json
+++ b/tests/Obsidian/__test_data__/multiple_headings.json
@@ -18,13 +18,13 @@
         }
       },
       {
-        "heading": "Level 2 heading",
+        "heading": "Level 2 heading with no taks",
         "level": 2,
         "position": {
           "end": {
-            "col": 18,
+            "col": 31,
             "line": 2,
-            "offset": 39
+            "offset": 52
           },
           "start": {
             "col": 0,
@@ -33,19 +33,35 @@
           }
         }
       },
+      {
+        "heading": "Level 2 heading",
+        "level": 2,
+        "position": {
+          "end": {
+            "col": 18,
+            "line": 4,
+            "offset": 72
+          },
+          "start": {
+            "col": 0,
+            "line": 4,
+            "offset": 54
+          }
+        }
+      },
       {
         "heading": "Level 3 heading",
         "level": 3,
         "position": {
           "end": {
             "col": 19,
-            "line": 4,
-            "offset": 60
+            "line": 13,
+            "offset": 344
           },
           "start": {
             "col": 0,
-            "line": 4,
-            "offset": 41
+            "line": 13,
+            "offset": 325
           }
         }
       }
@@ -55,14 +71,78 @@
         "parent": -6,
         "position": {
           "end": {
-            "col": 39,
+            "col": 71,
             "line": 6,
-            "offset": 101
+            "offset": 145
           },
           "start": {
             "col": 0,
             "line": 6,
-            "offset": 62
+            "offset": 74
+          }
+        },
+        "task": " "
+      },
+      {
+        "parent": -6,
+        "position": {
+          "end": {
+            "col": 71,
+            "line": 7,
+            "offset": 217
+          },
+          "start": {
+            "col": 0,
+            "line": 7,
+            "offset": 146
+          }
+        },
+        "task": " "
+      },
+      {
+        "parent": -11,
+        "position": {
+          "end": {
+            "col": 71,
+            "line": 11,
+            "offset": 323
+          },
+          "start": {
+            "col": 0,
+            "line": 11,
+            "offset": 252
+          }
+        },
+        "task": " "
+      },
+      {
+        "parent": -15,
+        "position": {
+          "end": {
+            "col": 75,
+            "line": 15,
+            "offset": 421
+          },
+          "start": {
+            "col": 0,
+            "line": 15,
+            "offset": 346
+          }
+        },
+        "task": " "
+      },
+      {
+        "parent": -15,
+        "position": {
+          "end": {
+            "col": 75,
+            "line": 16,
+            "offset": 497
+          },
+          "start": {
+            "col": 0,
+            "line": 16,
+            "offset": 422
           }
         },
         "task": " "
@@ -87,9 +167,9 @@
       {
         "position": {
           "end": {
-            "col": 18,
+            "col": 31,
             "line": 2,
-            "offset": 39
+            "offset": 52
           },
           "start": {
             "col": 0,
@@ -102,14 +182,14 @@
       {
         "position": {
           "end": {
-            "col": 19,
+            "col": 18,
             "line": 4,
-            "offset": 60
+            "offset": 72
           },
           "start": {
             "col": 0,
             "line": 4,
-            "offset": 41
+            "offset": 54
           }
         },
         "type": "heading"
@@ -117,14 +197,74 @@
       {
         "position": {
           "end": {
-            "col": 39,
-            "line": 6,
-            "offset": 101
+            "col": 71,
+            "line": 7,
+            "offset": 217
           },
           "start": {
             "col": 0,
             "line": 6,
-            "offset": 62
+            "offset": 74
+          }
+        },
+        "type": "list"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 31,
+            "line": 9,
+            "offset": 250
+          },
+          "start": {
+            "col": 0,
+            "line": 9,
+            "offset": 219
+          }
+        },
+        "type": "paragraph"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 71,
+            "line": 11,
+            "offset": 323
+          },
+          "start": {
+            "col": 0,
+            "line": 11,
+            "offset": 252
+          }
+        },
+        "type": "list"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 19,
+            "line": 13,
+            "offset": 344
+          },
+          "start": {
+            "col": 0,
+            "line": 13,
+            "offset": 325
+          }
+        },
+        "type": "heading"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 75,
+            "line": 16,
+            "offset": 497
+          },
+          "start": {
+            "col": 0,
+            "line": 15,
+            "offset": 346
           }
         },
         "type": "list"
@@ -136,22 +276,85 @@
           "end": {
             "col": 11,
             "line": 6,
-            "offset": 73
+            "offset": 85
           },
           "start": {
             "col": 6,
             "line": 6,
-            "offset": 68
+            "offset": 80
+          }
+        },
+        "tag": "#task"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 11,
+            "line": 7,
+            "offset": 157
+          },
+          "start": {
+            "col": 6,
+            "line": 7,
+            "offset": 152
+          }
+        },
+        "tag": "#task"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 11,
+            "line": 11,
+            "offset": 263
+          },
+          "start": {
+            "col": 6,
+            "line": 11,
+            "offset": 258
+          }
+        },
+        "tag": "#task"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 11,
+            "line": 15,
+            "offset": 357
+          },
+          "start": {
+            "col": 6,
+            "line": 15,
+            "offset": 352
+          }
+        },
+        "tag": "#task"
+      },
+      {
+        "position": {
+          "end": {
+            "col": 11,
+            "line": 16,
+            "offset": 433
+          },
+          "start": {
+            "col": 6,
+            "line": 16,
+            "offset": 428
           }
         },
         "tag": "#task"
       }
-    ],
-    "v": 1
+    ]
   },
-  "fileContents": "# multiple_headings\n\n## Level 2 heading\n\n### Level 3 heading\n\n- [ ] #task Task in 'multiple_headings'\n",
+  "fileContents": "# multiple_headings\n\n## Level 2 heading with no taks\n\n## Level 2 heading\n\n- [ ] #task Task 1 in Level 2 heading - in 'multiple_headings' - list 1\n- [ ] #task task 2 in Level 2 heading - in 'multiple_headings' - list 1\n\nDivider to create a new section\n\n- [ ] #task task 3 in Level 2 heading - in 'multiple_headings' - list 2\n\n### Level 3 heading\n\n- [ ] #task Task 4 in 'multiple_headings' - in 'multiple_headings' - list 3\n- [ ] #task Task 5 in 'multiple_headings' - in 'multiple_headings' - list 3\n",
   "filePath": "Test Data/multiple_headings.md",
   "getAllTags": [
+    "#task",
+    "#task",
+    "#task",
+    "#task",
     "#task"
   ],
   "obsidianApiVersion": "1.7.7",