Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1400 (2022-12-15 21:30:05)

refactor: DueDateField now implements 'sort by due'
<!--- Provide a general summary of your changes in the Title above -->

# Description

- `DueDateField` now implements `sort by due`
- There is a lot of duplication of code now, which will be removed in a later step.

## Motivation and Context

This is step 3 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 0bd4e8ed..42cff751 100644
--- a/src/Query/Filter/DueDateField.ts
+++ b/src/Query/Filter/DueDateField.ts
@@ -1,5 +1,7 @@
 import type { Moment } from 'moment';
 import type { Task } from '../../Task';
+import type { Comparator } from '../Sort';
+import { Sort, Sorting } from '../Sort';
 import { DateField } from './DateField';
 
 /**
@@ -20,4 +22,59 @@ export class DueDateField extends DateField {
     protected filterResultIfFieldMissing() {
         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);
+    }
+
+    /**
+     * 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());
+    }
+
+    /**
+     * 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)
+        return (a: Task, b: Task) => {
+            return Sort.compareByDate(a.dueDate, b.dueDate);
+        };
+    }
 }
diff --git a/src/Query/Filter/StatusField.ts b/src/Query/Filter/StatusField.ts
index 63947e99..91f178a5 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/src/Query/Filter/StatusField.ts
@@ -18,19 +18,48 @@ export class StatusField extends FilterInstructionsBasedField {
     /**
      * Parse a sort instruction line, creating either a {@link Sorting} object or undefined,
      * if the line is unrecognised.
-     * TODO Separate the parsing of the line and construction of the {@link Sorting} in to two separate methods.
      * @param line - One of 'sort by status' or 'sort by status reverse'
      */
-    public createSorter(line: string): Sorting | undefined {
+    public parseInstructionAndCreateSorter(line: string): Sorting | undefined {
         const sortByRegexp = /^sort by (status)( reverse)?/;
         const fieldMatch = line.match(sortByRegexp);
         if (fieldMatch !== null) {
-            return new Sorting(!!fieldMatch[2], 1, fieldMatch[1], StatusField.comparator());
+            // 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);
+    }
+
+    /**
+     * 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());
+    }
+
     /**
      * Return a function to compare two Task objects, for use in sorting by status.
      */
diff --git a/src/Query/Query.ts b/src/Query/Query.ts
index 263f508c..8fa50471 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -8,20 +8,12 @@ import { parseFilter } 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.
 //      Then remove SortingProperty.
-export type SortingProperty =
-    | 'urgency'
-    | 'priority'
-    | 'start'
-    | 'scheduled'
-    | 'due'
-    | 'done'
-    | 'path'
-    | 'description'
-    | 'tag';
+export type SortingProperty = 'urgency' | 'priority' | 'start' | 'scheduled' | 'done' | 'path' | 'description' | 'tag';
 
 export type GroupingProperty =
     | 'backlink'
@@ -55,7 +47,7 @@ export class Query implements IQuery {
     // If a tag is specified the user can also add a number to specify
     // which one to sort by if there is more than one.
     private readonly sortByRegexp =
-        /^sort by (urgency|status|priority|start|scheduled|due|done|path|description|tag)( reverse)?[\s]*(\d+)?/;
+        /^sort by (urgency|priority|start|scheduled|done|path|description|tag)( reverse)?[\s]*(\d+)?/;
 
     private readonly groupByRegexp =
         /^group by (backlink|done|due|filename|folder|happens|heading|path|priority|recurrence|recurring|root|scheduled|start|status|tags)/;
@@ -90,6 +82,8 @@ export class Query implements IQuery {
                     case this.sortByRegexp.test(line):
                         this.parseSortBy({ line });
                         break;
+                    case this.parseSortBy2({ line }):
+                        break;
                     case this.groupByRegexp.test(line):
                         this.parseGroupBy({ line });
                         break;
@@ -236,18 +230,30 @@ export class Query implements IQuery {
         }
     }
 
-    private parseSortBy({ line }: { line: string }): void {
+    private parseSortBy2({ line }: { line: string }): boolean {
         // New style parsing, which is done by the Field classes.
-        // Initially this is only implemented for status.
+        // 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 statusSorter = new StatusField().createSorter(line);
-        if (statusSorter) {
-            this._sorting.push(statusSorter);
-            return;
+        {
+            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;
+            }
         }
+        return false;
+    }
 
+    private parseSortBy({ line }: { line: string }): void {
         // Old-style parsing, based on all the values in SortingProperty above.
         // TODO Migrate all the compare functions in Sort over to the
         //      corresponding fields, so that the block below can be removed.
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index ef537ba2..3d45ae5d 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -2,7 +2,9 @@ import type moment from 'moment';
 import type { Task } from '../Task';
 import { getSettings } from '../Config/Settings';
 import type { Query, SortingProperty } from './Query';
-import { StatusField } from './Filter/StatusField'; // TODO Remove the cylcing dependency between StatusField and Sort.
+// TODO Remove the cyclic dependency between StatusField and Sort.
+import { StatusField } from './Filter/StatusField';
+import { DueDateField } from './Filter/DueDateField';
 
 /**
  * A sorting function, that takes two Task objects and returns
@@ -55,6 +57,9 @@ export class Sorting {
      */
     public makeComparator(reverse: boolean) {
         const comparator = Sort.comparators[this.property as SortingProperty];
+        if (!comparator) {
+            throw Error('Unrecognised legacy sort keyword: ' + this.property);
+        }
         return Sorting.maybeReverse(reverse, comparator);
     }
 
@@ -70,7 +75,7 @@ export class Sort {
         const defaultComparators: Comparator[] = [
             Sort.compareByUrgency,
             StatusField.comparator(),
-            Sort.compareByDueDate,
+            DueDateField.comparator(),
             Sort.compareByPriority,
             Sort.compareByPath,
         ];
@@ -93,7 +98,6 @@ export class Sort {
         priority: Sort.compareByPriority,
         start: Sort.compareByStartDate,
         scheduled: Sort.compareByScheduledDate,
-        due: Sort.compareByDueDate,
         done: Sort.compareByDoneDate,
         path: Sort.compareByPath,
         tag: Sort.compareByTag,
@@ -133,10 +137,6 @@ export class Sort {
         return Sort.compareByDate(a.scheduledDate, b.scheduledDate);
     }
 
-    private static compareByDueDate(a: Task, b: Task): -1 | 0 | 1 {
-        return Sort.compareByDate(a.dueDate, b.dueDate);
-    }
-
     private static compareByDoneDate(a: Task, b: Task): -1 | 0 | 1 {
         return Sort.compareByDate(a.doneDate, b.doneDate);
     }


Test patch:
diff --git a/tests/Query/Filter/DueDateField.test.ts b/tests/Query/Filter/DueDateField.test.ts
index c1d0ce48..fe83d268 100644
--- a/tests/Query/Filter/DueDateField.test.ts
+++ b/tests/Query/Filter/DueDateField.test.ts
@@ -55,3 +55,28 @@ describe('explain due date queries', () => {
         expect(filterOrMessage).toHaveExplanation('due date is on 2023-01-02 (Monday 2nd January 2023)');
     });
 });
+
+describe('sorting by status', () => {
+    const date1 = new TaskBuilder().dueDate('2021-01-12').build();
+    const date2 = new TaskBuilder().dueDate('2022-12-23').build();
+
+    it('sort by due', () => {
+        // Arrange
+        const sorter = new DueDateField().createNormalSorter();
+
+        // Assert
+        expect(sorter.comparator(date1, date2)).toEqual(-1);
+        expect(sorter.comparator(date2, date1)).toEqual(1);
+        expect(sorter.comparator(date2, date2)).toEqual(0);
+    });
+
+    it('sort by due reverse', () => {
+        // Arrange
+        const sorter = new DueDateField().createReverseSorter();
+
+        // Assert
+        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/StatusField.test.ts b/tests/Query/Filter/StatusField.test.ts
index 20c4b49d..a560ecb7 100644
--- a/tests/Query/Filter/StatusField.test.ts
+++ b/tests/Query/Filter/StatusField.test.ts
@@ -35,19 +35,17 @@ describe('sorting by status', () => {
 
     it('sort by status', () => {
         // Arrange
-        const sorter = new StatusField().createSorter('sort by status');
+        const sorter = new StatusField().createNormalSorter();
 
         // Assert
-        expect(sorter).toBeDefined();
-        expect(sorter!.comparator(doneTask, todoTask)).toEqual(1);
+        expect(sorter.comparator(doneTask, todoTask)).toEqual(1);
     });
 
     it('sort by status reverse', () => {
         // Arrange
-        const sorter = new StatusField().createSorter('sort by status reverse');
+        const sorter = new StatusField().createReverseSorter();
 
         // Assert
-        expect(sorter).toBeDefined();
-        expect(sorter!.comparator(doneTask, todoTask)).toEqual(-1);
+        expect(sorter.comparator(doneTask, todoTask)).toEqual(-1);
     });
 });
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index 4a47c221..40e762dd 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -11,6 +11,7 @@ import { resetSettings, updateSettings } from '../src/Config/Settings';
 import { DateParser } from '../src/Query/DateParser';
 import type { Task } from '../src/Task';
 import { StatusField } from '../src/Query/Filter/StatusField';
+import { DueDateField } from '../src/Query/Filter/DueDateField';
 import { fromLine } from './TestHelpers';
 import { TaskBuilder } from './TestingTools/TaskBuilder';
 
@@ -60,38 +61,6 @@ describe('Sort', () => {
     //      is in a Field class, and the Field's tests exercise the particular sorting.
     //      Then the only testing needed here will probably be the testing of composite sorting.
 
-    // TODO Replace this with something simpler but equivalent in DueDateField.test.ts.
-    it('sorts correctly by due', () => {
-        const one = fromLine({
-            line: '- [x] bring out the trash 📅 2021-09-12',
-            path: '',
-        });
-        const two = fromLine({
-            line: '- [ ] pet the cat 📅 2021-09-15',
-            path: '',
-        });
-        const three = fromLine({
-            line: '- [ ] pet the cat 📅 2021-09-18',
-            path: '',
-        });
-        expect(
-            Sort.by(
-                {
-                    sorting: [new Sorting(false, 1, 'due')],
-                },
-                [one, two, three],
-            ),
-        ).toEqual([one, two, three]);
-        expect(
-            Sort.by(
-                {
-                    sorting: [new Sorting(false, 1, 'due')],
-                },
-                [two, three, one],
-            ),
-        ).toEqual([one, two, three]);
-    });
-
     // TODO Replace this with something simpler but equivalent in DoneDateField.test.ts.
     it('sorts correctly by done', () => {
         const one = fromLine({
@@ -139,9 +108,9 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        new Sorting(false, 1, 'due'),
+                        new DueDateField().createNormalSorter(),
                         new Sorting(false, 1, 'path'),
-                        new StatusField().createSorter('sort by status')!, // TODO Remove the need to invent a line to write this test
+                        new StatusField().createNormalSorter(),
                     ],
                 },
                 [one, four, two, three],
@@ -221,8 +190,8 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        new StatusField().createSorter('sort by status reverse')!, // TODO Remove the need to invent a line to write this test
-                        new Sorting(true, 1, 'due'),
+                        new StatusField().createReverseSorter(),
+                        new DueDateField().createReverseSorter(),
                         new Sorting(false, 1, 'path'),
                     ],
                 },