Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1435 (2022-12-26 22:22:20)

refactor: Rename Sorting to Sorter - and minor tidying
<!--- Provide a general summary of your changes in the Title above -->

# Description

- Rename `Sorting` to `Sorter` - and update various variable names to match
- Rename `Query.parseSortBy2()` to `parseSortBy()` now the original `parseSortBy()` method has gone
- Simplified some code
- Addressed some TODOs left over from recent refactoring of the `sort by` code.

## Motivation and Context

Just tidying up.

## How has this been tested?

Lots of running of 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/DateField.ts b/src/Query/Filter/DateField.ts
index b2f3d186..68881d9f 100644
--- a/src/Query/Filter/DateField.ts
+++ b/src/Query/Filter/DateField.ts
@@ -2,7 +2,7 @@ import type { Moment } from 'moment';
 import type { Task } from '../../Task';
 import { DateParser } from '../DateParser';
 import { Explanation } from '../Explain/Explanation';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { Field } from './Field';
 import { Filter, FilterOrErrorMessage } from './Filter';
 import { FilterInstructions } from './FilterInstructions';
diff --git a/src/Query/Filter/DescriptionField.ts b/src/Query/Filter/DescriptionField.ts
index 61ffe153..6e6933bb 100644
--- a/src/Query/Filter/DescriptionField.ts
+++ b/src/Query/Filter/DescriptionField.ts
@@ -1,6 +1,6 @@
 import { getSettings } from '../../Config/Settings';
 import type { Task } from '../../Task';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { TextField } from './TextField';
 
 /**
diff --git a/src/Query/Filter/Field.ts b/src/Query/Filter/Field.ts
index c89b53bf..96cf3c8a 100644
--- a/src/Query/Filter/Field.ts
+++ b/src/Query/Filter/Field.ts
@@ -1,5 +1,5 @@
-import { Sorting } from '../Sorting';
-import type { Comparator } from '../Sorting';
+import { Sorter } from '../Sorter';
+import type { Comparator } from '../Sorter';
 import type { FilterOrErrorMessage } from './Filter';
 
 /**
@@ -104,20 +104,22 @@ export abstract class Field {
     // -----------------------------------------------------------------------------------------------------------------
 
     /**
-     * Return whether the code for this field implements sorting of tasks
+     * Return whether the code for this field implements sorting of tasks.
+     *
+     * If overriding this to return true, in order to enable sorting,
+     * the method ${@link comparator} must also be overridden.
      */
     public supportsSorting(): boolean {
-        // TODO Make abstract
         return false;
     }
 
     /**
-     * Parse a 'sort by' line and return a Sorting object.
+     * Parse a 'sort by' line and return a {@link Sorter} object.
      *
      * Returns null line does not match this field or is invalid,
      * or this field does not support sorting.
      */
-    public parseSortLine(line: string): Sorting | null {
+    public parseSortLine(line: string): Sorter | null {
         if (!this.supportsSorting()) {
             return null;
         }
@@ -126,11 +128,7 @@ export abstract class Field {
             return null;
         }
 
-        const sorting = this.createSorterFromLine(line);
-        if (sorting) {
-            return sorting;
-        }
-        return null;
+        return this.createSorterFromLine(line);
     }
 
     /**
@@ -151,7 +149,7 @@ export abstract class Field {
     }
 
     /**
-     * Parse the line, and return either a {@link Sorting} object or null.
+     * Parse the line, and return either a {@link Sorter} object or null.
      *
      * This default implementation works for all fields that support
      * the default sorting pattern of `sort by <fieldName> (reverse)?`.
@@ -163,7 +161,7 @@ export abstract class Field {
      *
      * @see {@link canCreateSorterForLine}
      */
-    public createSorterFromLine(line: string): Sorting | null {
+    public createSorterFromLine(line: string): Sorter | null {
         if (!this.supportsSorting()) {
             return null;
         }
@@ -198,37 +196,39 @@ export abstract class Field {
 
     /**
      * Return a function to compare two Task objects, for use in sorting by this field's value.
+     *
+     * See ${@link supportsSorting} for what to do, to enable support of sorting in a
+     * particular ${@link Field} implementation.
      */
     public comparator(): Comparator {
-        // TODO Make abstract
         throw Error(`comparator() unimplemented for ${this.fieldNameSingular()}`);
     }
 
     /**
-     * Create a {@link Sorting} object for sorting tasks by this field's value.
+     * Create a {@link Sorter} 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 {
-        return new Sorting(reverse, this.fieldNameSingular(), this.comparator());
+    public createSorter(reverse: boolean): Sorter {
+        return new Sorter(this.fieldNameSingular(), this.comparator(), reverse);
     }
 
     /**
-     * Create a {@link Sorting} object for sorting tasks by this field's value,
+     * Create a {@link Sorter} object for sorting tasks by this field's value,
      * in the standard/normal sort order for this field.
      *
      * @see {@link createReverseSorter}
      */
-    public createNormalSorter(): Sorting {
+    public createNormalSorter(): Sorter {
         return this.createSorter(false);
     }
 
     /**
-     * Create a {@link Sorting} object for sorting tasks by this field's value,
+     * Create a {@link Sorter} 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 {
+    public createReverseSorter(): Sorter {
         return this.createSorter(true);
     }
 }
diff --git a/src/Query/Filter/HappensDateField.ts b/src/Query/Filter/HappensDateField.ts
index 86137822..bf9cf187 100644
--- a/src/Query/Filter/HappensDateField.ts
+++ b/src/Query/Filter/HappensDateField.ts
@@ -2,7 +2,7 @@ import type { Moment } from 'moment';
 import type { Task } from '../../Task';
 import { DateParser } from '../DateParser';
 import { Explanation } from '../Explain/Explanation';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { Field } from './Field';
 import { Filter, FilterOrErrorMessage } from './Filter';
 import { FilterInstructions } from './FilterInstructions';
diff --git a/src/Query/Filter/PathField.ts b/src/Query/Filter/PathField.ts
index c018ab7d..b8dc28ca 100644
--- a/src/Query/Filter/PathField.ts
+++ b/src/Query/Filter/PathField.ts
@@ -1,5 +1,5 @@
 import type { Task } from '../../Task';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { TextField } from './TextField';
 
 /** Support the 'path' search instruction.
diff --git a/src/Query/Filter/PriorityField.ts b/src/Query/Filter/PriorityField.ts
index 07110d8d..344c056e 100644
--- a/src/Query/Filter/PriorityField.ts
+++ b/src/Query/Filter/PriorityField.ts
@@ -1,6 +1,6 @@
 import { Priority, Task } from '../../Task';
 import { Explanation } from '../Explain/Explanation';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { Field } from './Field';
 import { Filter, FilterOrErrorMessage } from './Filter';
 
diff --git a/src/Query/Filter/StatusField.ts b/src/Query/Filter/StatusField.ts
index 5d6a1df5..85dca098 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/src/Query/Filter/StatusField.ts
@@ -1,5 +1,5 @@
 import { Status, Task } from '../../Task';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { FilterInstructionsBasedField } from './FilterInstructionsBasedField';
 
 export class StatusField extends FilterInstructionsBasedField {
@@ -21,7 +21,6 @@ export class StatusField extends FilterInstructionsBasedField {
     /**
      * Return a function to compare two Task objects, for use in sorting by status.
      */
-    // TODO Make comparator() in Field through if unimplemented
     public comparator(): Comparator {
         return (a: Task, b: Task) => {
             if (a.status < b.status) {
diff --git a/src/Query/Filter/TagsField.ts b/src/Query/Filter/TagsField.ts
index e8f8ce39..0cb00244 100644
--- a/src/Query/Filter/TagsField.ts
+++ b/src/Query/Filter/TagsField.ts
@@ -1,6 +1,6 @@
 import type { Task } from '../../Task';
-import type { Comparator } from '../Sorting';
-import { Sorting } from '../Sorting';
+import type { Comparator } from '../Sorter';
+import { Sorter } from '../Sorter';
 import { MultiTextField } from './MultiTextField';
 
 /**
@@ -33,7 +33,7 @@ export class TagsField extends MultiTextField {
      *
      * @param line
      */
-    public createSorterFromLine(line: string): Sorting | null {
+    public createSorterFromLine(line: string): Sorter | null {
         const match = line.match(this.sorterRegExp());
         if (match === null) {
             return null;
@@ -42,7 +42,7 @@ export class TagsField extends MultiTextField {
         const reverse = !!match[1];
         const propertyInstance = isNaN(+match[2]) ? 1 : +match[2];
         const comparator = TagsField.makeCompareByTagComparator(propertyInstance);
-        return new Sorting(reverse, this.fieldNameSingular(), comparator);
+        return new Sorter(this.fieldNameSingular(), comparator, reverse);
     }
 
     /**
diff --git a/src/Query/Filter/TextField.ts b/src/Query/Filter/TextField.ts
index d2bfd383..4bf41a74 100644
--- a/src/Query/Filter/TextField.ts
+++ b/src/Query/Filter/TextField.ts
@@ -3,7 +3,7 @@ import { SubstringMatcher } from '../Matchers/SubstringMatcher';
 import { RegexMatcher } from '../Matchers/RegexMatcher';
 import type { IStringMatcher } from '../Matchers/IStringMatcher';
 import { Explanation } from '../Explain/Explanation';
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import { Field } from './Field';
 import type { FilterFunction } from './Filter';
 import { Filter, FilterOrErrorMessage } from './Filter';
diff --git a/src/Query/Filter/UrgencyField.ts b/src/Query/Filter/UrgencyField.ts
index 93bab692..a9ddb1b0 100644
--- a/src/Query/Filter/UrgencyField.ts
+++ b/src/Query/Filter/UrgencyField.ts
@@ -1,4 +1,4 @@
-import type { Comparator } from '../Sorting';
+import type { Comparator } from '../Sorter';
 import type { Task } from '../../Task';
 import { Field } from './Field';
 import { FilterOrErrorMessage } from './Filter';
diff --git a/src/Query/FilterParser.ts b/src/Query/FilterParser.ts
index cada02a1..f7bfaa05 100644
--- a/src/Query/FilterParser.ts
+++ b/src/Query/FilterParser.ts
@@ -16,7 +16,7 @@ import { FilenameField } from './Filter/FilenameField';
 import { UrgencyField } from './Filter/UrgencyField';
 
 import type { FilterOrErrorMessage } from './Filter/Filter';
-import type { Sorting } from './Sorting';
+import type { Sorter } from './Sorter';
 
 const fieldCreators = [
     () => new StatusField(),
@@ -45,7 +45,7 @@ export function parseFilter(filterString: string): FilterOrErrorMessage | null {
     return null;
 }
 
-export function parseSorter(sorterString: string): Sorting | null {
+export function parseSorter(sorterString: string): Sorter | null {
     // New style parsing, using sorting which is done by the Field classes.
 
     // Optimisation: Check whether line begins with 'sort by'
diff --git a/src/Query/Query.ts b/src/Query/Query.ts
index 136b8145..9a1a9098 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -3,7 +3,7 @@ import type { Task } from '../Task';
 import type { IQuery } from '../IQuery';
 import { getSettings } from '../Config/Settings';
 import { Sort } from './Sort';
-import type { Sorting } from './Sorting';
+import type { Sorter } from './Sorter';
 import type { TaskGroups } from './TaskGroups';
 import { parseFilter, parseSorter } from './FilterParser';
 import { Group } from './Group';
@@ -35,7 +35,7 @@ export class Query implements IQuery {
     private _layoutOptions: LayoutOptions = new LayoutOptions();
     private _filters: Filter[] = [];
     private _error: string | undefined = undefined;
-    private _sorting: Sorting[] = [];
+    private _sorting: Sorter[] = [];
     private _grouping: Grouping[] = [];
 
     private readonly groupByRegexp =
@@ -68,7 +68,7 @@ export class Query implements IQuery {
                     case this.limitRegexp.test(line):
                         this.parseLimit({ line });
                         break;
-                    case this.parseSortBy2({ line }):
+                    case this.parseSortBy({ line }):
                         break;
                     case this.groupByRegexp.test(line):
                         this.parseGroupBy({ line });
@@ -216,7 +216,7 @@ export class Query implements IQuery {
         }
     }
 
-    private parseSortBy2({ line }: { line: string }): boolean {
+    private parseSortBy({ line }: { line: string }): boolean {
         const sortingMaybe = parseSorter(line);
         if (sortingMaybe) {
             this._sorting.push(sortingMaybe);
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index e356a4b1..8e5ef4d4 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -1,5 +1,5 @@
 import type { Task } from '../Task';
-import type { Comparator } from './Sorting';
+import type { Comparator } from './Sorter';
 import type { Query } from './Query';
 // TODO Remove the cyclic dependency between StatusField and Sort.
 import { StatusField } from './Filter/StatusField';
diff --git a/src/Query/Sorting.ts b/src/Query/Sorter.ts
similarity index 72%
rename from src/Query/Sorting.ts
rename to src/Query/Sorter.ts
index 5afc7212..5ab233f9 100644
--- a/src/Query/Sorting.ts
+++ b/src/Query/Sorter.ts
@@ -7,35 +7,33 @@ import type { Task } from '../Task';
  * - `+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.
+ * Typically Comparator functions are stored in a {@link Sorter} object.
  */
 export type Comparator = (a: Task, b: Task) => number;
 
 /**
- * Sorting represents a single 'sort by' instruction.
+ * Sorter represents a single 'sort by' instruction.
  * It stores the comparison function as a {@link Comparator}.
  */
-export class Sorting {
+export class Sorter {
     public readonly property: string;
     public readonly comparator: Comparator;
 
     /**
      * 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 property - the name of the property.
      * @param comparator - {@link Comparator} function, for sorting in the standard direction.
      *                     If `reverse` is true, it will automatically be converted to reverse the sort direction.
+     * @param reverse - whether the sort order should be reversed.
      */
-    constructor(reverse: boolean, property: string, comparator: Comparator) {
+    constructor(property: string, comparator: Comparator, reverse: boolean) {
         this.property = property;
-        this.comparator = Sorting.maybeReverse(reverse, comparator);
+        this.comparator = Sorter.maybeReverse(reverse, comparator);
     }
 
     private static maybeReverse(reverse: boolean, comparator: Comparator) {
-        return reverse ? Sorting.makeReversedComparator(comparator) : comparator;
+        return reverse ? Sorter.makeReversedComparator(comparator) : comparator;
     }
 
     private static makeReversedComparator(comparator: Comparator): Comparator {


Test patch:
diff --git a/tests/CustomMatchers/CustomMatchersForSorting.ts b/tests/CustomMatchers/CustomMatchersForSorting.ts
index 8f6d1699..f5a18b8b 100644
--- a/tests/CustomMatchers/CustomMatchersForSorting.ts
+++ b/tests/CustomMatchers/CustomMatchersForSorting.ts
@@ -1,7 +1,7 @@
 import type moment from 'moment';
 import { DateParser } from '../../src/Query/DateParser';
 import { DateField } from '../../src/Query/Filter/DateField';
-import type { Sorting } from '../../src/Query/Sorting';
+import type { Sorter } from '../../src/Query/Sorter';
 import type { Task } from '../../src/Task';
 
 declare global {
@@ -97,21 +97,21 @@ function testCompareByDateBothWays(dateA: string | null, dateB: string | null, e
 // Sorting Tasks
 // ---------------------------------------------------------------------
 
-export function expectTaskComparesBefore(sorting: Sorting, taskA: Task, taskB: Task) {
-    testCompareTasksBothWays(sorting, taskA, taskB, before);
+export function expectTaskComparesBefore(sorter: Sorter, taskA: Task, taskB: Task) {
+    testCompareTasksBothWays(sorter, taskA, taskB, before);
 }
 
-export function expectTaskComparesEqual(sorting: Sorting, taskA: Task, taskB: Task) {
-    testCompareTasksBothWays(sorting, taskA, taskB, equal);
+export function expectTaskComparesEqual(sorter: Sorter, taskA: Task, taskB: Task) {
+    testCompareTasksBothWays(sorter, taskA, taskB, equal);
 }
 
-export function expectTaskComparesAfter(sorting: Sorting, taskA: Task, taskB: Task) {
-    testCompareTasksBothWays(sorting, taskA, taskB, after);
+export function expectTaskComparesAfter(sorter: Sorter, taskA: Task, taskB: Task) {
+    testCompareTasksBothWays(sorter, taskA, taskB, after);
 }
 
-function testCompareTasksBothWays(sorting: Sorting, taskA: Task, taskB: Task, expected: -1 | 0 | 1) {
-    expect({ sorting, tasks: [taskA, taskB] }).toCompareTasksWithResult(expected);
+function testCompareTasksBothWays(sorter: Sorter, taskA: Task, taskB: Task, expected: -1 | 0 | 1) {
+    expect({ sorting: sorter, tasks: [taskA, taskB] }).toCompareTasksWithResult(expected);
 
     const reverseExpected = expected === equal ? equal : -expected;
-    expect({ sorting, tasks: [taskB, taskA] }).toCompareTasksWithResult(reverseExpected);
+    expect({ sorting: sorter, tasks: [taskB, taskA] }).toCompareTasksWithResult(reverseExpected);
 }
diff --git a/tests/Query/Filter/DescriptionField.test.ts b/tests/Query/Filter/DescriptionField.test.ts
index b41ca7cd..ba92e228 100644
--- a/tests/Query/Filter/DescriptionField.test.ts
+++ b/tests/Query/Filter/DescriptionField.test.ts
@@ -263,7 +263,7 @@ describe('sorting by description', () => {
         return new TaskBuilder().description(description).build();
     }
 
-    it('sort by path', () => {
+    it('sort by description', () => {
         // Arrange
         const sorter = new DescriptionField().createNormalSorter();
 
@@ -273,7 +273,7 @@ describe('sorting by description', () => {
         expectTaskComparesAfter(sorter, with_description('AAA'), with_description('aaa')); // case-sensitive - capitals come last - TODO WHY? In description searches, capitals appear to come first.
     });
 
-    it('sort by path reverse', () => {
+    it('sort by description reverse', () => {
         // Single example just to prove reverse works.
         // (There's no need to repeat all the examples above)
         const sorter = new DescriptionField().createReverseSorter();
diff --git a/tests/Query/Filter/Field.test.ts b/tests/Query/Filter/Field.test.ts
index 7613a4bf..e1d8e2fc 100644
--- a/tests/Query/Filter/Field.test.ts
+++ b/tests/Query/Filter/Field.test.ts
@@ -1,6 +1,6 @@
 import { Field } from '../../../src/Query/Filter/Field';
 import type { FilterOrErrorMessage } from '../../../src/Query/Filter/Filter';
-import type { Comparator } from '../../../src/Query/Sorting';
+import type { Comparator } from '../../../src/Query/Sorter';
 import type { Task } from '../../../src/Task';
 import { expectTaskComparesBefore } from '../../CustomMatchers/CustomMatchersForSorting';
 import { TaskBuilder } from '../../TestingTools/TaskBuilder';
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index e2e4a3a5..d1e8a3be 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -5,9 +5,9 @@ import moment from 'moment';
 
 window.moment = moment;
 
-import type { Comparator } from '../src/Query/Sorting';
+import type { Comparator } from '../src/Query/Sorter';
 import { Sort } from '../src/Query/Sort';
-import { Sorting } from '../src/Query/Sorting';
+import { Sorter } from '../src/Query/Sorter';
 import type { Task } from '../src/Task';
 import { StatusField } from '../src/Query/Filter/StatusField';
 import { DoneDateField } from '../src/Query/Filter/DoneDateField';
@@ -38,7 +38,7 @@ describe('Sort', () => {
 
         // Normal way round
         {
-            const sortByDescriptionLength = new Sorting(false, 'junk', comparator);
+            const sortByDescriptionLength = new Sorter('junk', comparator, false);
             expect(sortByDescriptionLength.comparator(short, long)).toEqual(1);
             expect(sortByDescriptionLength.comparator(short, short)).toEqual(0);
             expect(sortByDescriptionLength.comparator(long, short)).toEqual(-1);
@@ -46,7 +46,7 @@ describe('Sort', () => {
 
         // Reversed
         {
-            const sortByDescriptionLength = new Sorting(true, 'junk', comparator);
+            const sortByDescriptionLength = new Sorter('junk', comparator, true);
             expect(sortByDescriptionLength.comparator(short, long)).toEqual(-1);
             expect(sortByDescriptionLength.comparator(short, short)).toEqual(-0);
             expect(sortByDescriptionLength.comparator(long, short)).toEqual(1);