Merge pull request #1977 from obsidian-tasks-group/simplify-sorting-code

refactor: Simplify sorting code in Field.ts

Author: claremacrae

src/Query/Filter/Field.ts

@@ -131,41 +131,6 @@ export abstract class Field {
         return false;
     }
 
-    /**
-     * 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): Sorter | null {
-        if (!this.supportsSorting()) {
-            return null;
-        }
-
-        if (!this.canCreateSorterForLine(line)) {
-            return null;
-        }
-
-        return this.createSorterFromLine(line);
-    }
-
-    /**
-     * Returns true if the class can parse the given 'sort by' instruction line.
-     *
-     * Current implementation simply checks whether the class does support sorting,
-     * and whether the line matches this.sorterRegExp().
-     * @param line - A line from a ```tasks``` block.
-     *
-     * @see {@link createSorterFromLine}
-     */
-    public canCreateSorterForLine(line: string): boolean {
-        if (!this.supportsSorting()) {
-            return false;
-        }
-
-        return Field.lineMatchesFilter(this.sorterRegExp(), line);
-    }
-
     /**
      * Parse the line, and return either a {@link Sorter} object or null.
      *
@@ -176,8 +141,6 @@ export abstract class Field {
      * this method.
      *
      * @param line - A 'sort by' line from a ```tasks``` block.
-     *
-     * @see {@link canCreateSorterForLine}
      */
     public createSorterFromLine(line: string): Sorter | null {
         if (!this.supportsSorting()) {

src/Query/FilterParser.ts

@@ -81,7 +81,7 @@ export function parseSorter(sorterString: string): Sorter | null {
     // See if any of the fields can parse the line.
     for (const creator of fieldCreators) {
         const field = creator();
-        const sorter = field.parseSortLine(sorterString);
+        const sorter = field.createSorterFromLine(sorterString);
         if (sorter) {
             return sorter;
         }

tests/Query/Filter/Field.test.ts

@@ -62,11 +62,8 @@ describe('sorting - base class usability and implementation', () => {
         });
 
         it('should fail to parse a "valid" line', () => {
-            // expect(unsupported.parseSortLine('sort by unsupported')).toThrow(Error);
             const line = 'sort by unsupported';
-            expect(unsupported.canCreateSorterForLine(line)).toBe(false);
-            expect(unsupported.createSorterFromLine(line)).toBeNull();
-            const sorting = unsupported.parseSortLine(line);
+            const sorting = unsupported.createSorterFromLine(line);
             expect(sorting).toBeNull();
         });
     });
@@ -84,23 +81,19 @@ describe('sorting - base class usability and implementation', () => {
 
         it('should parse a valid line', () => {
             const line = 'sort by description-length';
-            expect(supported.canCreateSorterForLine(line)).toBe(true);
-            expect(supported.createSorterFromLine(line)).not.toBeNull();
-            const sorting = supported.parseSortLine(line);
+            const sorting = supported.createSorterFromLine(line);
             expect(sorting).not.toBeNull();
             expect(sorting?.property).toEqual('description-length');
         });
 
         it('should fail to parse a invalid line', () => {
             const line = 'sort by jsdajhasdfa';
-            expect(supported.canCreateSorterForLine(line)).toBe(false);
-            expect(supported.createSorterFromLine(line)).toBeNull();
-            const sorting = supported.parseSortLine(line);
+            const sorting = supported.createSorterFromLine(line);
             expect(sorting).toBeNull();
         });
 
         it('should compare two tasks', () => {
-            const sorting = supported.parseSortLine('sort by description-length');
+            const sorting = supported.createSorterFromLine('sort by description-length');
             const a = new TaskBuilder().description('short description').build();
             const b = new TaskBuilder().description('very looooooooong description').build();
             expectTaskComparesBefore(sorting!, a, b);

tests/Query/Filter/RecurringField.test.ts

@@ -60,7 +60,7 @@ describe('sorting by recurring', () => {
 
     it('parses sort by recurrence', () => {
         const field = new RecurringField();
-        expect(field.parseSortLine('sort by recurring')).not.toBeNull();
+        expect(field.createSorterFromLine('sort by recurring')).not.toBeNull();
     });
 
     it('sort by due', () => {

tests/Query/Filter/TagsField.test.ts

@@ -345,17 +345,6 @@ describe('Sort by tags', () => {
             expect(tagsField.supportsSorting()).toEqual(true);
         });
 
-        it('should report whether it can parse lines', () => {
-            // Valid sort by tag lines:
-            expect(tagsField.canCreateSorterForLine('sort by tag')).toBe(true);
-            expect(tagsField.canCreateSorterForLine('sort by tag 2')).toBe(true);
-            expect(tagsField.canCreateSorterForLine('sort by tag reverse')).toBe(true);
-            expect(tagsField.canCreateSorterForLine('sort by tag reverse 3')).toBe(true);
-
-            // Invalid lines:
-            expect(tagsField.canCreateSorterForLine('sort by description')).toBe(false);
-        });
-
         const tag_a = new TaskBuilder().tags(['#a']).build();
         const tag_b = new TaskBuilder().tags(['#b']).build();
         const tags_a_b = new TaskBuilder().tags(['#a', '#b']).build();
@@ -373,20 +362,20 @@ describe('Sort by tags', () => {
         });
 
         it('should parse a valid line with default tag number', () => {
-            const sorter = tagsField.parseSortLine('sort by tag');
+            const sorter = tagsField.createSorterFromLine('sort by tag');
             expect(sorter?.property).toEqual('tag');
             expect(sorter?.comparator(tag_a, tag_b)).toBeLessThan(0);
             expectTaskComparesBefore(sorter!, tag_a, tag_b);
         });
 
         it('should parse a valid line with a non-default tag number', () => {
-            const sorter = tagsField.parseSortLine('sort by tag 2');
+            const sorter = tagsField.createSorterFromLine('sort by tag 2');
             expect(sorter?.property).toEqual('tag');
             expectTaskComparesBefore(sorter!, tags_a_b, tags_a_c);
         });
 
         it('should parse a valid line with reverse and a non-default tag number', () => {
-            const sorter = tagsField.parseSortLine('sort by tag reverse 2');
+            const sorter = tagsField.createSorterFromLine('sort by tag reverse 2');
             expect(sorter?.property).toEqual('tag');
             expectTaskComparesAfter(sorter!, tags_a_b, tags_a_c);
         });
@@ -406,9 +395,7 @@ describe('Sort by tags', () => {
 
         it('should fail to parse a invalid line', () => {
             const line = 'sort by jsdajhasdfa';
-            expect(tagsField.canCreateSorterForLine(line)).toBe(false);
-            expect(tagsField.createSorterFromLine(line)).toBeNull();
-            const sorting = tagsField.parseSortLine(line);
+            const sorting = tagsField.createSorterFromLine(line);
             expect(sorting).toBeNull();
         });
     });
@@ -429,7 +416,10 @@ describe('Sort by tags', () => {
 
         // Act / Assert
         expect(
-            Sort.by([new TagsField().parseSortLine('sort by tag 1')!], [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10]),
+            Sort.by(
+                [new TagsField().createSorterFromLine('sort by tag 1')!],
+                [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
+            ),
         ).toEqual(expectedOrder);
     });
 
@@ -450,7 +440,7 @@ describe('Sort by tags', () => {
         // Act / Assert
         expect(
             Sort.by(
-                [new TagsField().parseSortLine('sort by tag reverse 1')!],
+                [new TagsField().createSorterFromLine('sort by tag reverse 1')!],
                 [t1, t3, t5, t7, t6, t4, t2, t8, t9, t10],
             ),
         ).toEqual(expectedOrder);
@@ -463,7 +453,9 @@ 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([new TagsField().parseSortLine('sort by tag 2')!], [t4, t3, t2, t1, t5])).toEqual(expectedOrder);
+        expect(Sort.by([new TagsField().createSorterFromLine('sort by tag 2')!], [t4, t3, t2, t1, t5])).toEqual(
+            expectedOrder,
+        );
     });
 
     it('should sort correctly reversed by second tag with no global filter', () => {
@@ -473,7 +465,7 @@ 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([new TagsField().parseSortLine('sort by tag reverse 2')!], [t4, t3, t2, t1, t5])).toEqual(
+        expect(Sort.by([new TagsField().createSorterFromLine('sort by tag reverse 2')!], [t4, t3, t2, t1, t5])).toEqual(
             expectedOrder,
         );
     });
@@ -501,7 +493,7 @@ describe('Sort by tags', () => {
         // Act
         expect(
             Sort.by(
-                [new TagsField().parseSortLine('sort by tag 1')!],
+                [new TagsField().createSorterFromLine('sort by tag 1')!],
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
         ).toEqual(expectedOrder);
@@ -533,7 +525,7 @@ describe('Sort by tags', () => {
         // Act
         expect(
             Sort.by(
-                [new TagsField().parseSortLine('sort by tag reverse 1')!],
+                [new TagsField().createSorterFromLine('sort by tag reverse 1')!],
                 [t1, t12, t3, t13, t5, t7, t6, t4, t2, t8, t9, t10, t11],
             ),
         ).toEqual(expectedOrder);
@@ -557,7 +549,10 @@ describe('Sort by tags', () => {
         const expectedOrder = [t1, t2, t3, t4, t5, t6, t7, t8];
 
         // Act
-        const result = Sort.by([new TagsField().parseSortLine('sort by tag 2')!], [t4, t7, t5, t2, t3, t1, t8, t6]);
+        const result = Sort.by(
+            [new TagsField().createSorterFromLine('sort by tag 2')!],
+            [t4, t7, t5, t2, t3, t1, t8, t6],
+        );
 
         // Assert
         expect(result).toEqual(expectedOrder);
@@ -582,7 +577,7 @@ describe('Sort by tags', () => {
 
         // Act
         const result = Sort.by(
-            [new TagsField().parseSortLine('sort by tag reverse 2')!],
+            [new TagsField().createSorterFromLine('sort by tag reverse 2')!],
             [t4, t7, t5, t2, t3, t1, t8, t6],
         );
 
@@ -620,8 +615,8 @@ describe('Sort by tags', () => {
         expect(
             Sort.by(
                 [
-                    new TagsField().parseSortLine('sort by tag 2')!, // tag 2 - ascending
-                    new TagsField().parseSortLine('sort by tag 1')!, // tag 1 - ascending
+                    new TagsField().createSorterFromLine('sort by tag 2')!, // tag 2 - ascending
+                    new TagsField().createSorterFromLine('sort by tag 1')!, // tag 1 - ascending
                     new DescriptionField().createNormalSorter(), // then description - ascending
                 ],
                 input,