Merge pull request #3224 from obsidian-tasks-group/refactor-FileParser

claremacrae · web-flow · commit 4a7a5305c6e4 · 2024-12-06T13:04:12.000Z
refactor: Introduce FileParser class
diff --git 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/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
@@ -8,118 +8,151 @@ 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,
                             parent: parentListItem,
                         });
                     }
 
-                    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
@@ -85,4 +85,9 @@ export class TaskLocation {
     public get hasKnownPath(): boolean {
         return this.path !== '';
     }
+
+    public allFieldsExceptTasksFileForTesting() {
+        const { _tasksFile, ...rest } = { ...this };
+        return rest;
+    }
 }
diff --git a/tests/Obsidian/FileParser.test.ts 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
@@ -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,14 +18,15 @@ 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!,
         logger,
         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

Original file line number	Diff line number	Diff line change
`@@ -85,4 +85,9 @@ export class TaskLocation {`
`85`	`85`	`public get hasKnownPath(): boolean {`
`86`	`86`	`return this.path !== '';`
`87`	`87`	`}`
	`88`	`+`
	`89`	`+ public allFieldsExceptTasksFileForTesting() {`
	`90`	`+ const { _tasksFile, ...rest } = { ...this };`
	`91`	`+ return rest;`
	`92`	`+ }`
`88`	`93`	`}`