Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1436 (2022-12-26 23:04:16)

refactor: Remove cyclic dependency between Query and Sort
<!--- Provide a general summary of your changes in the Title above -->

# Description

Remove cyclic dependency between Query and Sort.

This simplifies the code for calling `Sort.by()`, by allowing a simple list of `Sorter` objects to be passed in, instead of requiring an object that contains a sorting that gives a list of `Sorter` objects.

## Motivation and Context

Mainly I wanted to remove one of the 15 current cyclic dependencies in `src/`.

However, another benefit is that the code for calling `Sort.by()` is now a lot less opaque.

## How has this been tested?

By running all the 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)

## 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/Query.ts b/src/Query/Query.ts
index 9a1a9098..fbbb24c5 100644
--- a/src/Query/Query.ts
+++ b/src/Query/Query.ts
@@ -149,7 +149,7 @@ export class Query implements IQuery {
             tasks = tasks.filter(filter.filterFunction);
         });
 
-        const tasksSortedLimited = Sort.by(this, tasks).slice(0, this.limit);
+        const tasksSortedLimited = Sort.by(this.sorting, tasks).slice(0, this.limit);
         return Group.by(this.grouping, tasksSortedLimited);
     }
 
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index 8e5ef4d4..b660e72b 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -1,7 +1,6 @@
 import type { Task } from '../Task';
 import type { Comparator } from './Sorter';
-import type { Query } from './Query';
-// TODO Remove the cyclic dependency between StatusField and Sort.
+import type { Sorter } from './Sorter';
 import { StatusField } from './Filter/StatusField';
 import { DueDateField } from './Filter/DueDateField';
 import { PriorityField } from './Filter/PriorityField';
@@ -9,8 +8,7 @@ import { PathField } from './Filter/PathField';
 import { UrgencyField } from './Filter/UrgencyField';
 
 export class Sort {
-    public static by(query: Pick<Query, 'sorting'>, tasks: Task[]): Task[] {
-        // TODO Move code for creating default comparators to separate file
+    public static by(sorters: Sorter[], tasks: Task[]) {
         const defaultComparators: Comparator[] = [
             new UrgencyField().comparator(),
             new StatusField().comparator(),
@@ -21,8 +19,8 @@ export class Sort {
 
         const userComparators: Comparator[] = [];
 
-        for (const sorting of query.sorting) {
-            userComparators.push(sorting.comparator);
+        for (const sorter of sorters) {
+            userComparators.push(sorter.comparator);
         }
 
         return tasks.sort(Sort.makeCompositeComparator([...userComparators, ...defaultComparators]));


Test patch:
diff --git a/tests/Query/Filter/DescriptionField.test.ts b/tests/Query/Filter/DescriptionField.test.ts
index ba92e228..f8bbcea7 100644
--- a/tests/Query/Filter/DescriptionField.test.ts
+++ b/tests/Query/Filter/DescriptionField.test.ts
@@ -360,13 +360,8 @@ describe('sorting by description', () => {
         });
 
         const expectedOrder = [one, two, three, four, five];
-        expect(
-            Sort.by(
-                {
-                    sorting: [new DescriptionField().createNormalSorter()],
-                },
-                [two, one, five, four, three],
-            ),
-        ).toEqual(expectedOrder);
+        expect(Sort.by([new DescriptionField().createNormalSorter()], [two, one, five, four, three])).toEqual(
+            expectedOrder,
+        );
     });
 });
diff --git a/tests/Query/Filter/TagsField.test.ts b/tests/Query/Filter/TagsField.test.ts
index f689980c..3085d4bf 100644
--- a/tests/Query/Filter/TagsField.test.ts
+++ b/tests/Query/Filter/TagsField.test.ts
@@ -352,12 +352,7 @@ describe('Sort by tags', () => {
 
         // Act / Assert
         expect(
-            Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag 1')!],
-                },
-                [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
-            ),
+            Sort.by([new TagsField().parseSortLine('sort by tag 1')!], [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10]),
         ).toEqual(expectedOrder);
     });
 
@@ -378,9 +373,7 @@ describe('Sort by tags', () => {
         // Act / Assert
         expect(
             Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag reverse 1')!],
-                },
+                [new TagsField().parseSortLine('sort by tag reverse 1')!],
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
         ).toEqual(expectedOrder);
@@ -393,14 +386,7 @@ describe('Sort by tags', () => {
         const t4 = fromLine({ line: '- [ ] a #iii #ddd' });
         const t5 = fromLine({ line: '- [ ] a #hhh #eee' });
         const expectedOrder = [t1, t2, t3, t4, t5];
-        expect(
-            Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag 2')!],
-                },
-                [t4, t3, t2, t1, t5],
-            ),
-        ).toEqual(expectedOrder);
+        expect(Sort.by([new TagsField().parseSortLine('sort by tag 2')!], [t4, t3, t2, t1, t5])).toEqual(expectedOrder);
     });
 
     it('should sort correctly reversed by second tag with no global filter', () => {
@@ -410,14 +396,9 @@ describe('Sort by tags', () => {
         const t4 = fromLine({ line: '- [ ] a #iii #ddd' });
         const t5 = fromLine({ line: '- [ ] a #hhh #eee' });
         const expectedOrder = [t5, t4, t3, t2, t1];
-        expect(
-            Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag reverse 2')!],
-                },
-                [t4, t3, t2, t1, t5],
-            ),
-        ).toEqual(expectedOrder);
+        expect(Sort.by([new TagsField().parseSortLine('sort by tag reverse 2')!], [t4, t3, t2, t1, t5])).toEqual(
+            expectedOrder,
+        );
     });
 
     it('should sort correctly by tag defaulting to first with global filter', () => {
@@ -443,9 +424,7 @@ describe('Sort by tags', () => {
         // Act
         expect(
             Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag 1')!],
-                },
+                [new TagsField().parseSortLine('sort by tag 1')!],
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
         ).toEqual(expectedOrder);
@@ -477,9 +456,7 @@ describe('Sort by tags', () => {
         // Act
         expect(
             Sort.by(
-                {
-                    sorting: [new TagsField().parseSortLine('sort by tag reverse 1')!],
-                },
+                [new TagsField().parseSortLine('sort by tag reverse 1')!],
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
         ).toEqual(expectedOrder);
@@ -503,12 +480,7 @@ describe('Sort by tags', () => {
         const expectedOrder = [t1, t2, t3, t4, t5, t6, t7, t8];
 
         // Act
-        const result = Sort.by(
-            {
-                sorting: [new TagsField().parseSortLine('sort by tag 2')!],
-            },
-            [t4, t7, t5, t2, t3, t1, t8, t6],
-        );
+        const result = Sort.by([new TagsField().parseSortLine('sort by tag 2')!], [t4, t7, t5, t2, t3, t1, t8, t6]);
 
         // Assert
         expect(result).toEqual(expectedOrder);
@@ -533,9 +505,7 @@ describe('Sort by tags', () => {
 
         // Act
         const result = Sort.by(
-            {
-                sorting: [new TagsField().parseSortLine('sort by tag reverse 2')!],
-            },
+            [new TagsField().parseSortLine('sort by tag reverse 2')!],
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );
 
@@ -572,13 +542,11 @@ describe('Sort by tags', () => {
         ];
         expect(
             Sort.by(
-                {
-                    sorting: [
-                        new TagsField().parseSortLine('sort by tag 2')!, // tag 2 - ascending
-                        new TagsField().parseSortLine('sort by tag 1')!, // tag 1 - ascending
-                        new DescriptionField().createNormalSorter(), // then description - ascending
-                    ],
-                },
+                [
+                    new TagsField().parseSortLine('sort by tag 2')!, // tag 2 - ascending
+                    new TagsField().parseSortLine('sort by tag 1')!, // tag 1 - ascending
+                    new DescriptionField().createNormalSorter(), // then description - ascending
+                ],
                 input,
             ),
         ).toEqual(correctExpectedOrder);
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index d1e8a3be..a0e6fdb1 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -61,7 +61,7 @@ describe('Sort', () => {
         const five = fromLine({ line: '- [x] b 📅 1970-01-02', path: '3' });
         const six = fromLine({ line: '- [x] d 📅 1970-01-03', path: '2' });
         const expectedOrder = [one, two, three, four, five, six];
-        expect(Sort.by({ sorting: [] }, [six, five, one, four, two, three])).toEqual(expectedOrder);
+        expect(Sort.by([], [six, five, one, four, two, three])).toEqual(expectedOrder);
     });
 
     // TODO Add some tests based on easy-to-reason-about custom comparators, then delete the remaining old-style tests
@@ -85,13 +85,11 @@ describe('Sort', () => {
         ];
         expect(
             Sort.by(
-                {
-                    sorting: [
-                        new DueDateField().createNormalSorter(),
-                        new PathField().createNormalSorter(),
-                        new StatusField().createNormalSorter(),
-                    ],
-                },
+                [
+                    new DueDateField().createNormalSorter(),
+                    new PathField().createNormalSorter(),
+                    new StatusField().createNormalSorter(),
+                ],
                 [one, four, two, three],
             ),
         ).toEqual(expectedOrder);
@@ -118,9 +116,7 @@ describe('Sort', () => {
         const expectedOrder = [one, two, three, four];
         expect(
             Sort.by(
-                {
-                    sorting: [new DescriptionField().createNormalSorter(), new DoneDateField().createNormalSorter()],
-                },
+                [new DescriptionField().createNormalSorter(), new DoneDateField().createNormalSorter()],
                 [three, one, two, four],
             ),
         ).toEqual(expectedOrder);
@@ -147,9 +143,7 @@ describe('Sort', () => {
         const expectedOrder = [one, two, three, four];
         expect(
             Sort.by(
-                {
-                    sorting: [new DescriptionField().createReverseSorter(), new DoneDateField().createNormalSorter()],
-                },
+                [new DescriptionField().createReverseSorter(), new DoneDateField().createNormalSorter()],
                 [two, four, three, one],
             ),
         ).toEqual(expectedOrder);
@@ -167,13 +161,11 @@ describe('Sort', () => {
 
         expect(
             Sort.by(
-                {
-                    sorting: [
-                        new StatusField().createReverseSorter(),
-                        new DueDateField().createReverseSorter(),
-                        new PathField().createNormalSorter(),
-                    ],
-                },
+                [
+                    new StatusField().createReverseSorter(),
+                    new DueDateField().createReverseSorter(),
+                    new PathField().createNormalSorter(),
+                ],
                 [six, five, one, four, three, two],
             ),
         ).toEqual(expectedOrder);