Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1399 (2022-12-15 18:00:59)

refactor: StatusField now implements 'sort by status'
<!--- Provide a general summary of your changes in the Title above -->

# Description

- StatusField now implements 'sort by status'
- Lots of comments added to show where I'm going with this series of refactorings.
- Removed a test of parsing of sort instructions which was totally focused on implementation details, and was fragile when Sorting's fields changed. This will be replaced by later individual tests of each sort function in each Field.

## Motivation and Context

This is step 2 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/StatusField.ts b/src/Query/Filter/StatusField.ts
index ebc7e83b..63947e99 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/src/Query/Filter/StatusField.ts
@@ -1,4 +1,6 @@
 import { Status, Task } from '../../Task';
+import type { Comparator } from '../Sort';
+import { Sorting } from '../Sort';
 import { FilterInstructionsBasedField } from './FilterInstructionsBasedField';
 
 export class StatusField extends FilterInstructionsBasedField {
@@ -12,4 +14,35 @@ export class StatusField extends FilterInstructionsBasedField {
     public fieldName(): string {
         return 'status';
     }
+
+    /**
+     * 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 {
+        const sortByRegexp = /^sort by (status)( reverse)?/;
+        const fieldMatch = line.match(sortByRegexp);
+        if (fieldMatch !== null) {
+            return new Sorting(!!fieldMatch[2], 1, fieldMatch[1], StatusField.comparator());
+        } else {
+            return undefined;
+        }
+    }
+
+    /**
+     * Return a function to compare two Task objects, for use in sorting by status.
+     */
+    public static comparator(): Comparator {
+        return (a: Task, b: Task) => {
+            if (a.status < b.status) {
+                return 1;
+            } else if (a.status > b.status) {
+                return -1;
+            } else {
+                return 0;
+            }
+        };
+    }
 }
diff --git a/src/Query/Query.ts b/src/Query/Query.ts
index d8676c5f..263f508c 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -7,10 +7,13 @@ import type { TaskGroups } from './TaskGroups';
 import { parseFilter } from './FilterParser';
 import { Group } from './Group';
 import type { Filter } from './Filter/Filter';
+import { StatusField } from './Filter/StatusField';
 
+// TODO Work through the values in SortingProperty, moving their comparison
+//      functions to the corresponding Field implementations.
+//      Then remove SortingProperty.
 export type SortingProperty =
     | 'urgency'
-    | 'status'
     | 'priority'
     | 'start'
     | 'scheduled'
@@ -234,13 +237,27 @@ export class Query implements IQuery {
     }
 
     private parseSortBy({ line }: { line: string }): void {
+        // New style parsing, which is done by the Field classes.
+        // Initially this is only implemented for status.
+        // 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;
+        }
+
+        // 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.
         const fieldMatch = line.match(this.sortByRegexp);
         if (fieldMatch !== null) {
             this._sorting.push(
                 new Sorting(
-                    fieldMatch[1] as SortingProperty,
                     !!fieldMatch[2],
                     isNaN(+fieldMatch[3]) ? 1 : +fieldMatch[3],
+                    fieldMatch[1] as SortingProperty,
                 ),
             );
         } else {
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index 56dfe0d7..ef537ba2 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -2,25 +2,64 @@ import type moment from 'moment';
 import type { Task } from '../Task';
 import { getSettings } from '../Config/Settings';
 import type { Query, SortingProperty } from './Query';
-
-type Comparator = (a: Task, b: Task) => number;
-
+import { StatusField } from './Filter/StatusField'; // TODO Remove the cylcing dependency between StatusField and Sort.
+
+/**
+ * A sorting function, that takes two Task objects and returns
+ * and returns one of:
+ * - `-1` or some other negative number, if a is less than b by some ordering criterion.
+ * - `+1` or some other positive number, if a is greater than b by the ordering criterion.
+ * - `0` or sometimes `-0`, if a equals b by the ordering criterion.
+ *
+ * Typically Comparator functions are stored in a {@link Sorting} object.
+ */
+export type Comparator = (a: Task, b: Task) => number;
+
+/**
+ * Sorting represents a single 'sort by' instruction.
+ * It stores the comparison function as a {@link Comparator}.
+ */
 export class Sorting {
-    public readonly property: SortingProperty;
+    public readonly property: string;
     public readonly comparator: Comparator;
-    public readonly reverse: boolean;
     public readonly propertyInstance: number;
 
-    constructor(property: SortingProperty, reverse: boolean, propertyInstance: number) {
+    /**
+     * Constructor.
+     *
+     * TODO Once SortingProperty has been removed, re-order the parameters so the comparator comes first.
+     *
+     * @param reverse - whether the sort order should be reversed.
+     * @param propertyInstance - for tag sorting, this is a 1-based index for the tag number in the task to sort by.
+     *                           TODO eventually, move this number to the comparator used for sorting by tag.
+     * @param property - the name of the property. If {@link comparator} is not supplied, this string must match
+     *                   one of the values in ${@link SortingProperty}.
+     * @param comparator - optional {@link Comparator} function. This will eventually become required, and will then be moved to
+     *                     the first parameter.
+     */
+    constructor(reverse: boolean, propertyInstance: number, property: string, comparator?: Comparator) {
         this.property = property;
-        this.reverse = reverse;
         this.propertyInstance = propertyInstance;
-        this.comparator = this.makeComparator();
+        if (comparator) {
+            this.comparator = Sorting.maybeReverse(reverse, comparator);
+        } else {
+            this.comparator = this.makeComparator(reverse);
+        }
+    }
+
+    /**
+     * Legacy function, for creating a Comparator for a SortingProperty value.
+     *
+     * TODO Once SortingProperty in Query.ts has been removed, remove this method.
+     * @param reverse
+     */
+    public makeComparator(reverse: boolean) {
+        const comparator = Sort.comparators[this.property as SortingProperty];
+        return Sorting.maybeReverse(reverse, comparator);
     }
 
-    public makeComparator() {
-        const comparator = Sort.comparators[this.property];
-        return this.reverse ? Sort.makeReversedComparator(comparator) : comparator;
+    private static maybeReverse(reverse: boolean, comparator: Comparator) {
+        return reverse ? Sort.makeReversedComparator(comparator) : comparator;
     }
 }
 
@@ -30,7 +69,7 @@ export class Sort {
     public static by(query: Pick<Query, 'sorting'>, tasks: Task[]): Task[] {
         const defaultComparators: Comparator[] = [
             Sort.compareByUrgency,
-            Sort.compareByStatus,
+            StatusField.comparator(),
             Sort.compareByDueDate,
             Sort.compareByPriority,
             Sort.compareByPath,
@@ -57,11 +96,11 @@ export class Sort {
         due: Sort.compareByDueDate,
         done: Sort.compareByDoneDate,
         path: Sort.compareByPath,
-        status: Sort.compareByStatus,
         tag: Sort.compareByTag,
     };
 
     public static makeReversedComparator(comparator: Comparator): Comparator {
+        // Note: This can return -0.
         return (a, b) => (comparator(a, b) * -1) as -1 | 0 | 1;
     }
 
@@ -82,16 +121,6 @@ export class Sort {
         return b.urgency - a.urgency;
     }
 
-    private static compareByStatus(a: Task, b: Task): -1 | 0 | 1 {
-        if (a.status < b.status) {
-            return 1;
-        } else if (a.status > b.status) {
-            return -1;
-        } else {
-            return 0;
-        }
-    }
-
     private static compareByPriority(a: Task, b: Task): number {
         return a.priority.localeCompare(b.priority);
     }


Test patch:
diff --git a/tests/Query.test.ts b/tests/Query.test.ts
index 87457f61..e3a06df7 100644
--- a/tests/Query.test.ts
+++ b/tests/Query.test.ts
@@ -5,10 +5,10 @@ import moment from 'moment';
 import { Query } from '../src/Query/Query';
 import { Priority, Status, Task } from '../src/Task';
 import { resetSettings, updateSettings } from '../src/Config/Settings';
-import { Sorting } from '../src/Query/Sort';
 import { createTasksFromMarkdown, fromLine } from './TestHelpers';
 import { shouldSupportFiltering } from './TestingTools/FilterTestHelpers';
 import type { FilteringCase } from './TestingTools/FilterTestHelpers';
+import { TaskBuilder } from './TestingTools/TaskBuilder';
 
 window.moment = moment;
 
@@ -710,31 +710,26 @@ describe('Query', () => {
         });
     });
 
-    describe('sorting instructions', () => {
-        const cases: { input: string; output: Sorting[] }[] = [
-            {
-                input: 'sort by status',
-                output: [new Sorting('status', false, 1)],
-            },
-            {
-                input: 'sort by status\nsort by due',
-                output: [new Sorting('status', false, 1), new Sorting('due', false, 1)],
-            },
-            {
-                input: 'sort by tag',
-                output: [new Sorting('tag', false, 1)],
-            },
-            {
-                input: 'sort by tag 2',
-                output: [new Sorting('tag', false, 2)],
-            },
-        ];
-        it.concurrent.each(cases)('sorting as %j', ({ input, output }) => {
-            const query = new Query({ source: input });
-
-            expect(query.sorting).toEqual(output);
+    describe('sorting', () => {
+        const doneTask = new TaskBuilder().status(Status.DONE).build();
+        const todoTask = new TaskBuilder().status(Status.TODO).build();
+
+        it('sort reverse returns -0 for equal tasks', () => {
+            // This test was added when I discovered that reverse sort returns
+            // -0 for equivalent tasks.
+            // This is a test to demonstrate that current behevaiour,
+            // rather than a test of the **required** behaviour.
+            // If the behaviour changes and '0' is returned instead of '-0',
+            // that is absolutely fine.
+            const query = new Query({ source: 'sort by status reverse' });
+            const sorter = query.sorting[0];
+
+            expect(sorter!.comparator(todoTask, doneTask)).toEqual(1);
+            expect(sorter!.comparator(doneTask, doneTask)).toEqual(-0); // Note the minus sign. It's a consequence of
+            expect(sorter!.comparator(doneTask, todoTask)).toEqual(-1);
         });
     });
+
     describe('comments', () => {
         it('ignores comments', () => {
             // Arrange
diff --git a/tests/Query/Filter/StatusField.test.ts b/tests/Query/Filter/StatusField.test.ts
index 30dcfa1f..20c4b49d 100644
--- a/tests/Query/Filter/StatusField.test.ts
+++ b/tests/Query/Filter/StatusField.test.ts
@@ -28,3 +28,26 @@ describe('status', () => {
         testStatusFilter(filter, Status.DONE, false);
     });
 });
+
+describe('sorting by status', () => {
+    const doneTask = new TaskBuilder().status(Status.DONE).build();
+    const todoTask = new TaskBuilder().status(Status.TODO).build();
+
+    it('sort by status', () => {
+        // Arrange
+        const sorter = new StatusField().createSorter('sort by status');
+
+        // Assert
+        expect(sorter).toBeDefined();
+        expect(sorter!.comparator(doneTask, todoTask)).toEqual(1);
+    });
+
+    it('sort by status reverse', () => {
+        // Arrange
+        const sorter = new StatusField().createSorter('sort by status reverse');
+
+        // Assert
+        expect(sorter).toBeDefined();
+        expect(sorter!.comparator(doneTask, todoTask)).toEqual(-1);
+    });
+});
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index ac5b1d8f..4a47c221 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -5,12 +5,46 @@ import moment from 'moment';
 
 window.moment = moment;
 
+import type { Comparator } from '../src/Query/Sort';
 import { Sort, Sorting } from '../src/Query/Sort';
 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 { fromLine } from './TestHelpers';
+import { TaskBuilder } from './TestingTools/TaskBuilder';
 
 describe('Sort', () => {
+    it('constructs Sorting both ways from Comparator function', () => {
+        const comparator: Comparator = (a: Task, b: Task) => {
+            if (a.description.length < b.description.length) {
+                return 1;
+            } else if (a.description.length > b.description.length) {
+                return -1;
+            } else {
+                return 0;
+            }
+        };
+        const short = new TaskBuilder().description('short').build();
+        const long = new TaskBuilder().description('longer description').build();
+
+        // Normal way round
+        {
+            const sortByDescriptionLength = new Sorting(false, 1, 'junk', comparator);
+            expect(sortByDescriptionLength.comparator(short, long)).toEqual(1);
+            expect(sortByDescriptionLength.comparator(short, short)).toEqual(0);
+            expect(sortByDescriptionLength.comparator(long, short)).toEqual(-1);
+        }
+
+        // Reversed
+        {
+            const sortByDescriptionLength = new Sorting(true, 1, 'junk', comparator);
+            expect(sortByDescriptionLength.comparator(short, long)).toEqual(-1);
+            expect(sortByDescriptionLength.comparator(short, short)).toEqual(-0);
+            expect(sortByDescriptionLength.comparator(long, short)).toEqual(1);
+        }
+    });
+
     it('sorts correctly by default order', () => {
         const one = fromLine({ line: '- [ ] a 📅 1970-01-01', path: '3' });
         const two = fromLine({ line: '- [ ] c 📅 1970-01-02', path: '3' });
@@ -22,6 +56,11 @@ describe('Sort', () => {
         expect(Sort.by({ sorting: [] }, [six, five, one, four, two, three])).toEqual(expectedOrder);
     });
 
+    // TODO Most of these will become redundant once each of the sort implementations
+    //      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',
@@ -38,7 +77,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('due', false, 1)],
+                    sorting: [new Sorting(false, 1, 'due')],
                 },
                 [one, two, three],
             ),
@@ -46,13 +85,14 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('due', false, 1)],
+                    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({
             line: '- [x] pet the cat 📅 2021-09-15 ✅ 2021-09-15',
@@ -69,7 +109,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('done', false, 1)],
+                    sorting: [new Sorting(false, 1, 'done')],
                 },
                 [three, two, one],
             ),
@@ -77,7 +117,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('done', false, 1)],
+                    sorting: [new Sorting(false, 1, 'done')],
                 },
                 [two, one, three],
             ),
@@ -99,9 +139,9 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        new Sorting('due', false, 1),
-                        new Sorting('path', false, 1),
-                        new Sorting('status', false, 1),
+                        new Sorting(false, 1, 'due'),
+                        new Sorting(false, 1, 'path'),
+                        new StatusField().createSorter('sort by status')!, // TODO Remove the need to invent a line to write this test
                     ],
                 },
                 [one, four, two, three],
@@ -109,6 +149,7 @@ describe('Sort', () => {
         ).toEqual(expectedOrder);
     });
 
+    // TODO Replace this with something simpler but equivalent in DescriptionField.test.ts.
     it('sorts correctly by description, done', () => {
         const one = fromLine({
             line: '- [ ] a 📅 1970-01-02 ✅ 1971-01-01',
@@ -130,13 +171,14 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('description', false, 1), new Sorting('done', false, 1)],
+                    sorting: [new Sorting(false, 1, 'description'), new Sorting(false, 1, 'done')],
                 },
                 [three, one, two, four],
             ),
         ).toEqual(expectedOrder);
     });
 
+    // TODO Replace this with something simpler but equivalent in DescriptionField.test.ts.
     it('sorts correctly by description reverse, done', () => {
         const one = fromLine({
             line: '- [ ] b 📅 1970-01-01 ✅ 1971-01-01',
@@ -158,7 +200,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('description', true, 1), new Sorting('done', false, 1)],
+                    sorting: [new Sorting(true, 1, 'description'), new Sorting(false, 1, 'done')],
                 },
                 [two, four, three, one],
             ),
@@ -179,9 +221,9 @@ describe('Sort', () => {
             Sort.by(
                 {
                     sorting: [
-                        new Sorting('status', true, 1),
-                        new Sorting('due', true, 1),
-                        new Sorting('path', false, 1),
+                        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 Sorting(false, 1, 'path'),
                     ],
                 },
                 [six, five, one, four, three, two],
@@ -189,6 +231,7 @@ describe('Sort', () => {
         ).toEqual(expectedOrder);
     });
 
+    // TODO Replace this with something simpler but equivalent in DescriptionField.test.ts.
     it('sorts correctly by the link name and not the markdown', () => {
         const one = fromLine({
             line: '- [ ] *ZZZ An early task that starts with an A; actually not italic since only one asterisk',
@@ -210,7 +253,7 @@ describe('Sort', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('description', false, 1)],
+                    sorting: [new Sorting(false, 1, 'description')],
                 },
                 [two, one, five, four, three],
             ),
@@ -249,6 +292,7 @@ declare global {
 }
 
 // These are lower-level tests that the Task-based ones above, for ease of test coverage.
+// TODO Replace this with something simpler but equivalent in the tests for DateField.test.ts.
 describe('compareBy', () => {
     it('compares correctly by date', () => {
         const equal = 0;
@@ -287,6 +331,7 @@ describe('compareBy', () => {
  * There is also a task with additional characters in the name to ensure it is seen
  * as bigger that one with the same initial characters.
  */
+// TODO Replace this with something simpler but equivalent in TagsField.test.ts.
 describe('Sort by tags', () => {
     it('should sort correctly by tag defaulting to first with no global filter', () => {
         // Arrange
@@ -306,7 +351,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', false, 1)],
+                    sorting: [new Sorting(false, 1, 'tag')],
                 },
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
@@ -331,7 +376,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', true, 1)],
+                    sorting: [new Sorting(true, 1, 'tag')],
                 },
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
@@ -348,7 +393,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', false, 2)],
+                    sorting: [new Sorting(false, 2, 'tag')],
                 },
                 [t4, t3, t2, t1, t5],
             ),
@@ -365,7 +410,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', true, 2)],
+                    sorting: [new Sorting(true, 2, 'tag')],
                 },
                 [t4, t3, t2, t1, t5],
             ),
@@ -396,7 +441,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', false, 1)],
+                    sorting: [new Sorting(false, 1, 'tag')],
                 },
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
@@ -430,7 +475,7 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 {
-                    sorting: [new Sorting('tag', true, 1)],
+                    sorting: [new Sorting(true, 1, 'tag')],
                 },
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
@@ -457,7 +502,7 @@ describe('Sort by tags', () => {
         // Act
         const result = Sort.by(
             {
-                sorting: [new Sorting('tag', false, 2)],
+                sorting: [new Sorting(false, 2, 'tag')],
             },
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );
@@ -486,7 +531,7 @@ describe('Sort by tags', () => {
         // Act
         const result = Sort.by(
             {
-                sorting: [new Sorting('tag', true, 2)],
+                sorting: [new Sorting(true, 2, 'tag')],
             },
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );