Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1401 (2022-12-16 07:17:06)

refactor: Reduce repetition in new sorting code
<!--- Provide a general summary of your changes in the Title above -->

# Description

- Add Sorting interface and implementations to `Field`
    - Most of Sorting-related methods in `DueDateField` and `StatusField` now not needed
- `FilterParser` code for parsing a sort instruction is now a lot simpler

The next stage will be breaking cyclic links between files, as this is presently a lot of the next refactoring stages.

## Motivation and Context

This is step 4 in splitting out all the sort functions to the relevant fields, to make it easier to add new sort instructions, and to be able to re-use the sort code to sort groups in reverse order.

## How has this been tested?

- By running existing tests
- By adding new tests

## 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://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#updating-documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#maintaining-the-tests).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md) 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://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)
- [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/DueDateField.ts b/src/Query/Filter/DueDateField.ts
index 42cff751..f50540ac 100644
--- a/src/Query/Filter/DueDateField.ts
+++ b/src/Query/Filter/DueDateField.ts
@@ -23,58 +23,20 @@ export class DueDateField extends DateField {
         return false;
     }
 
-    /**
-     * Parse a sort instruction line, creating either a {@link Sorting} object or undefined,
-     * if the line is unrecognised.
-     * @param line - One of 'sort by due' or 'sort by due reverse'
-     */
-    public parseInstructionAndCreateSorter(line: string): Sorting | undefined {
-        const sortByRegexp = /^sort by (due)( reverse)?/;
-        const fieldMatch = line.match(sortByRegexp);
-        if (fieldMatch !== null) {
-            // const propertyName = fieldMatch[1];
-            const reverse = !!fieldMatch[2];
-            return this.createSorter(reverse);
-        } else {
-            return undefined;
-        }
-    }
-
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status,
-     * in the standard/normal sort order for this field.
-     *
-     * @see {@link createReverseSorter}
-     */
-    public createNormalSorter(): Sorting {
-        return this.createSorter(false);
+    public supportsSorting(): boolean {
+        return true;
     }
 
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status,
-     * in the reverse of the standard/normal sort order for this field.
-     *
-     * @see {@link createNormalSorter}
-     */
-    public createReverseSorter(): Sorting {
-        return this.createSorter(true);
-    }
-
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status.
-     * @param reverse - false for normal sort order, true for reverse sort order.
-     */
-    protected createSorter(reverse: boolean): Sorting {
-        return new Sorting(reverse, 1, 'due', DueDateField.comparator());
+    public createSorter(reverse: boolean): Sorting {
+        return new Sorting(reverse, 1, this.fieldName(), this.comparator());
     }
 
     /**
      * Return a function to compare two Task objects, for use in sorting by due.
      */
-    public static comparator(): Comparator {
-        // TODO Refactor to make non-static and use this.date(a), this.date(b)
+    public comparator(): Comparator {
         return (a: Task, b: Task) => {
-            return Sort.compareByDate(a.dueDate, b.dueDate);
+            return Sort.compareByDate(this.date(a), this.date(b));
         };
     }
 }
diff --git a/src/Query/Filter/Field.ts b/src/Query/Filter/Field.ts
index 5d242dbb..95d37ee9 100644
--- a/src/Query/Filter/Field.ts
+++ b/src/Query/Filter/Field.ts
@@ -1,3 +1,4 @@
+import type { Sorting } from '../Sort';
 import type { FilterOrErrorMessage } from './Filter';
 
 /**
@@ -77,4 +78,41 @@ export abstract class Field {
      * @public
      */
     public abstract fieldName(): string;
+
+    /**
+     * Return whether the code for this field implements sorting of tasks
+     */
+    public supportsSorting(): boolean {
+        // TODO Make abstract
+        return false;
+    }
+
+    /**
+     * Create a {@link Sorting} object for sorting tasks by this field's value.
+     * @param reverse - false for normal sort order, true for reverse sort order.
+     */
+    public createSorter(reverse: boolean): Sorting {
+        // TODO Make abstract
+        throw Error(`createSorter() unimplemented for ${this.fieldName()} - reverse = ${reverse}`);
+    }
+
+    /**
+     * Create a {@link Sorting} object for sorting tasks by this field's value,
+     * in the standard/normal sort order for this field.
+     *
+     * @see {@link createReverseSorter}
+     */
+    public createNormalSorter(): Sorting {
+        return this.createSorter(false);
+    }
+
+    /**
+     * Create a {@link Sorting} object for sorting tasks by this field's value,
+     * in the reverse of the standard/normal sort order for this field.
+     *
+     * @see {@link createNormalSorter}
+     */
+    public createReverseSorter(): Sorting {
+        return this.createSorter(true);
+    }
 }
diff --git a/src/Query/Filter/StatusField.ts b/src/Query/Filter/StatusField.ts
index 91f178a5..8f7a724c 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/src/Query/Filter/StatusField.ts
@@ -15,55 +15,20 @@ export class StatusField extends FilterInstructionsBasedField {
         return 'status';
     }
 
-    /**
-     * Parse a sort instruction line, creating either a {@link Sorting} object or undefined,
-     * if the line is unrecognised.
-     * @param line - One of 'sort by status' or 'sort by status reverse'
-     */
-    public parseInstructionAndCreateSorter(line: string): Sorting | undefined {
-        const sortByRegexp = /^sort by (status)( reverse)?/;
-        const fieldMatch = line.match(sortByRegexp);
-        if (fieldMatch !== null) {
-            // const propertyName = fieldMatch[1];
-            const reverse = !!fieldMatch[2];
-            return this.createSorter(reverse);
-        } else {
-            return undefined;
-        }
-    }
-
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status,
-     * in the standard/normal sort order for this field.
-     *
-     * @see {@link createReverseSorter}
-     */
-    public createNormalSorter(): Sorting {
-        return this.createSorter(false);
+    public supportsSorting(): boolean {
+        return true;
     }
 
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status,
-     * in the reverse of the standard/normal sort order for this field.
-     *
-     * @see {@link createNormalSorter}
-     */
-    public createReverseSorter(): Sorting {
-        return this.createSorter(true);
-    }
-
-    /**
-     * Create a {@link Sorting} object for sorting tasks by their Status.
-     * @param reverse - false for normal sort order, true for reverse sort order.
-     */
-    protected createSorter(reverse: boolean): Sorting {
-        return new Sorting(reverse, 1, 'status', StatusField.comparator());
+    // TODO Move createSorter() to Field
+    public createSorter(reverse: boolean): Sorting {
+        return new Sorting(reverse, 1, this.fieldName(), this.comparator());
     }
 
     /**
      * Return a function to compare two Task objects, for use in sorting by status.
      */
-    public static comparator(): Comparator {
+    // TODO Make comparator() in Field through if unimplemented
+    public comparator(): Comparator {
         return (a: Task, b: Task) => {
             if (a.status < b.status) {
                 return 1;
diff --git a/src/Query/FilterParser.ts b/src/Query/FilterParser.ts
index 773440f0..b12d4357 100644
--- a/src/Query/FilterParser.ts
+++ b/src/Query/FilterParser.ts
@@ -15,6 +15,7 @@ import { BooleanField } from './Filter/BooleanField';
 import { FilenameField } from './Filter/FilenameField';
 
 import type { FilterOrErrorMessage } from './Filter/Filter';
+import type { Sorting } from './Sort';
 
 const fieldCreators = [
     () => new StatusField(),
@@ -41,3 +42,26 @@ export function parseFilter(filterString: string): FilterOrErrorMessage | null {
     }
     return null;
 }
+
+export function parseSorter(sorterString: string): Sorting | null {
+    // New style parsing, using sorting which is done by the Field classes.
+    const sortByRegexp = /^sort by (\S+)( reverse)?/;
+    const fieldMatch = sorterString.match(sortByRegexp);
+    if (fieldMatch === null) {
+        return null;
+    }
+    const propertyName = fieldMatch[1];
+    const reverse = !!fieldMatch[2];
+
+    for (const creator of fieldCreators) {
+        const field = creator();
+        if (field.fieldName() === propertyName) {
+            if (field.supportsSorting()) {
+                return field.createSorter(reverse);
+            } else {
+                return null;
+            }
+        }
+    }
+    return null;
+}
diff --git a/src/Query/Query.ts b/src/Query/Query.ts
index 8fa50471..e0407304 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -4,11 +4,9 @@ import type { IQuery } from '../IQuery';
 import { getSettings } from '../Config/Settings';
 import { Sort, Sorting } from './Sort';
 import type { TaskGroups } from './TaskGroups';
-import { parseFilter } from './FilterParser';
+import { parseFilter, parseSorter } from './FilterParser';
 import { Group } from './Group';
 import type { Filter } from './Filter/Filter';
-import { StatusField } from './Filter/StatusField';
-import { DueDateField } from './Filter/DueDateField';
 
 // TODO Work through the values in SortingProperty, moving their comparison
 //      functions to the corresponding Field implementations.
@@ -231,24 +229,10 @@ export class Query implements IQuery {
     }
 
     private parseSortBy2({ line }: { line: string }): boolean {
-        // New style parsing, which is done by the Field classes.
-        // Initially this is only implemented for a few fields.
-        // TODO Once a few more Field classes have comparator implementations,
-        //      convert this to look like Query.parseFilter(),
-        //      which will call a new function in FilterParser - parseSorter() or parseSortBy()
-        {
-            const sorter = new StatusField().parseInstructionAndCreateSorter(line);
-            if (sorter) {
-                this._sorting.push(sorter);
-                return true;
-            }
-        }
-        {
-            const sorter = new DueDateField().parseInstructionAndCreateSorter(line);
-            if (sorter) {
-                this._sorting.push(sorter);
-                return true;
-            }
+        const sortingMaybe = parseSorter(line);
+        if (sortingMaybe) {
+            this._sorting.push(sortingMaybe);
+            return true;
         }
         return false;
     }
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index 3d45ae5d..24fa8998 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -15,12 +15,14 @@ import { DueDateField } from './Filter/DueDateField';
  *
  * Typically Comparator functions are stored in a {@link Sorting} object.
  */
+// TODO Move to Sorting.ts
 export type Comparator = (a: Task, b: Task) => number;
 
 /**
  * Sorting represents a single 'sort by' instruction.
  * It stores the comparison function as a {@link Comparator}.
  */
+// TODO Move to Sorting.ts
 export class Sorting {
     public readonly property: string;
     public readonly comparator: Comparator;
@@ -45,6 +47,7 @@ export class Sorting {
         if (comparator) {
             this.comparator = Sorting.maybeReverse(reverse, comparator);
         } else {
+            // TODO Move comparator mandatory so can remove reference to this.makeComparator
             this.comparator = this.makeComparator(reverse);
         }
     }
@@ -56,6 +59,7 @@ export class Sorting {
      * @param reverse
      */
     public makeComparator(reverse: boolean) {
+        // TODO Move this to Sort class
         const comparator = Sort.comparators[this.property as SortingProperty];
         if (!comparator) {
             throw Error('Unrecognised legacy sort keyword: ' + this.property);
@@ -72,10 +76,11 @@ export class Sort {
     static tagPropertyInstance: number = 1;
 
     public static by(query: Pick<Query, 'sorting'>, tasks: Task[]): Task[] {
+        // TODO Move code for creating default comparators to separate file
         const defaultComparators: Comparator[] = [
             Sort.compareByUrgency,
-            StatusField.comparator(),
-            DueDateField.comparator(),
+            new StatusField().comparator(),
+            new DueDateField().comparator(),
             Sort.compareByPriority,
             Sort.compareByPath,
         ];


Test patch:
diff --git a/tests/Query/Filter/DueDateField.test.ts b/tests/Query/Filter/DueDateField.test.ts
index fe83d268..676303f1 100644
--- a/tests/Query/Filter/DueDateField.test.ts
+++ b/tests/Query/Filter/DueDateField.test.ts
@@ -60,11 +60,17 @@ describe('sorting by status', () => {
     const date1 = new TaskBuilder().dueDate('2021-01-12').build();
     const date2 = new TaskBuilder().dueDate('2022-12-23').build();
 
+    it('supports Field sorting methods correctly', () => {
+        const field = new DueDateField();
+        expect(field.supportsSorting()).toEqual(true);
+    });
+
     it('sort by due', () => {
         // Arrange
         const sorter = new DueDateField().createNormalSorter();
 
         // Assert
+        // TODO Create expressive Jest custom matchers for sorting tasks
         expect(sorter.comparator(date1, date2)).toEqual(-1);
         expect(sorter.comparator(date2, date1)).toEqual(1);
         expect(sorter.comparator(date2, date2)).toEqual(0);
diff --git a/tests/Query/Filter/PriorityField.test.ts b/tests/Query/Filter/PriorityField.test.ts
index 27138397..b2956ab6 100644
--- a/tests/Query/Filter/PriorityField.test.ts
+++ b/tests/Query/Filter/PriorityField.test.ts
@@ -123,3 +123,11 @@ describe('explain priority', () => {
         expect(filterOrMessage).toHaveExplanation('priority is high');
     });
 });
+
+describe('sorting by priority', () => {
+    it('does not yet support Field sorting methods', () => {
+        const field = new PriorityField();
+        // Not yet supported - TODO - rename this test when implementing Priority sorting
+        expect(field.supportsSorting()).toEqual(false);
+    });
+});
diff --git a/tests/Query/Filter/StatusField.test.ts b/tests/Query/Filter/StatusField.test.ts
index a560ecb7..89bf3d33 100644
--- a/tests/Query/Filter/StatusField.test.ts
+++ b/tests/Query/Filter/StatusField.test.ts
@@ -33,6 +33,11 @@ describe('sorting by status', () => {
     const doneTask = new TaskBuilder().status(Status.DONE).build();
     const todoTask = new TaskBuilder().status(Status.TODO).build();
 
+    it('supports Field sorting methods correctly', () => {
+        const field = new StatusField();
+        expect(field.supportsSorting()).toEqual(true);
+    });
+
     it('sort by status', () => {
         // Arrange
         const sorter = new StatusField().createNormalSorter();