Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d7fc0dc
test: document current group sorting of PriorityField
ilandikov May 20, 2023
1acce88
refactor: move localeCompare to groupSorter
ilandikov May 20, 2023
1e7e92c
refactor: move groupSorter to Field
ilandikov May 20, 2023
edca28f
refactor: rename sorter to comparator
ilandikov May 20, 2023
3f1cb45
refactor: move groupComparator() to the bottom
ilandikov May 20, 2023
5e827e4
refactor: custom groupComparator() in PriorityField
ilandikov May 20, 2023
e9355f2
refactor: add Priority.toNumber()
ilandikov May 20, 2023
d0a078f
feat: make Priority group names shorter
ilandikov May 20, 2023
090c195
refactor: rename types
ilandikov May 20, 2023
f61c819
fix: toNumber() default priority is 3
ilandikov May 20, 2023
1d29d1d
refactor: remove temp variable
ilandikov May 20, 2023
bf597ea
refactor: shorter import type
ilandikov May 21, 2023
fefd7b2
test: fix test description
ilandikov May 21, 2023
3180630
refactor: compare first tasks instead of group names
ilandikov May 21, 2023
11c12be
docs: update group names in docs
ilandikov May 22, 2023
b7f9f20
test: test priority group sorting
ilandikov May 22, 2023
67a5baf
docs: document updated urgency grouping behaviour
ilandikov May 22, 2023
65d132c
refactor: add DescriptionLengthGroupingfield for testing
ilandikov May 22, 2023
8c2adc3
fix: add defalt group comparator if Field does not support sorting
ilandikov May 22, 2023
75e6cf7
test: test default group comparator
ilandikov May 22, 2023
a5c389c
test: fix date to fix urgency calculation
ilandikov May 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions docs/Queries/Grouping.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,12 @@ For more information, including adding your own customised statuses, see [[Statu

1. `priority`
- The priority of the task, namely one of:
- `Priority 1: High`
- `Priority 2: Medium`
- `Priority 3: None`
- `Priority 4: Low`
- `High`
- `Medium`
- `None`
- `Low`
1. `urgency` ([[Urgency|urgency]])
- Currently, the groups run from the lowest urgency to highest.
- You can reverse this with `group by urgency reverse`.
- In a future release, the default group order will become from the highest urgency to lowest.
- From the highest urgency to lowest.
1. `recurring`
- Whether the task is recurring: either `Recurring` or `Not Recurring`.
1. `recurrence`
Expand Down
43 changes: 43 additions & 0 deletions src/Query/Filter/DescriptionLengthGroupingField.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import type { Task } from 'Task';
import type { GrouperFunction } from 'Query/Grouper';
import { Field } from './Field';
import { FilterOrErrorMessage } from './Filter';

/** This is a class for test purposes of a Field that supports grouping but not sorting
*/
export class DescriptionLengthGroupingfield extends Field {
protected filterRegExp(): RegExp | null {
throw new Error('No filtering for description length field');
}
public fieldName(): string {
return 'description length';
}

public value(task: Task): number {
return task.description.length;
}

public createFilterOrErrorMessage(line: string): FilterOrErrorMessage {
return FilterOrErrorMessage.fromError(line, 'description length field does not support filtering');
}

// -----------------------------------------------------------------------------------------------------------------
// Sorting
// -----------------------------------------------------------------------------------------------------------------

// Doesn't support sorting by default as in Field.ts

// -----------------------------------------------------------------------------------------------------------------
// Grouping
// -----------------------------------------------------------------------------------------------------------------

public supportsGrouping(): boolean {
return true;
}

public grouper(): GrouperFunction {
return (task: Task) => {
return [this.value(task).toString()];
};
}
}
22 changes: 21 additions & 1 deletion src/Query/Filter/Field.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { Task } from 'Task';
import { Sorter } from '../Sorter';
import type { Comparator } from '../Sorter';
import * as RegExpTools from '../../lib/RegExpTools';
Expand Down Expand Up @@ -360,7 +361,13 @@ export abstract class Field {
* @param reverse - false for normal group order, true for reverse group order.
*/
public createGrouper(reverse: boolean): Grouper {
return new Grouper(this.fieldNameSingular(), this.grouper(), reverse);
let defaultOrFieldComparator = this.defaultGroupComparator;

if (this.supportsSorting()) {
defaultOrFieldComparator = this.comparator();
}

return new Grouper(this.fieldNameSingular(), this.grouper(), reverse, defaultOrFieldComparator);
}

/**
Expand All @@ -382,4 +389,17 @@ export abstract class Field {
public createReverseGrouper(): Grouper {
return this.createGrouper(true);
}

private defaultGroupComparator: Comparator = (a: Task, b: Task) => {
const groupNamesA = this.grouper()(a);
const groupNamesB = this.grouper()(b);

for (let i = 0; i < groupNamesA.length; i++) {
// The containers are guaranteed to be identical sizes since we are calling the same grouper
return groupNamesA[i].localeCompare(groupNamesB[i], undefined, { numeric: true });
}

// identical if we reach here
return 0;
};
}
2 changes: 1 addition & 1 deletion src/Query/Filter/MultiTextField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export abstract class MultiTextField extends TextField {
* This overloads {@link Field.createGrouper} to put a plural field name in the {@link Grouper.property}.
*/
public createGrouper(reverse: boolean): Grouper {
return new Grouper(this.fieldNamePlural(), this.grouper(), reverse);
return new Grouper(this.fieldNamePlural(), this.grouper(), reverse, this.comparator());
Copy link
Collaborator

@claremacrae claremacrae May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with this code if the field does not implement sorting?

I expect that calling this.comparator() will then throw some kind of exception.

If you look at the Quick Reference table you will find a few such cases - such as recurrence or recurring (I forget which).

And certainly the new group by function will create a grouper that does not sort.

So... you could write a failing test using recurrence, but if it is taught to sort in future, that test will become useless.

So the testing trick here is to write a new test and have it use a simple implementation for Field that does grouping but not sorting. Perhaps DescriptionLengthGroupingfield. A field that is written purely to make testing easy and self-explanatory.

Then line 67 can be changed to only use this.comparator() if supportsSorting() is true, and if not, fall back on the old behaviour of sorting by group name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with this code if the field does not implement sorting?

I expect that calling this.comparator() will then throw some kind of exception.

Very good point, thanks.

So the testing trick here is to write a new test and have it use a simple implementation for Field that does grouping but not sorting. Perhaps DescriptionLengthGroupingfield. A field that is written purely to make testing easy and self-explanatory.

Yup, I did that now, this seems to be a good trade-off.

Then line 67 can be changed to only use this.comparator() if supportsSorting() is true, and if not, fall back on the old behaviour of sorting by group name.

On Field level I don't have access to the group names so I can't compare them in the fall back case. I'm not really sure how to solve this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I got this, but not sure whether it is a good solution. Something smells here, can't tell what exactly. Probably the whole direction of the development ㅠㅠ

}

protected grouperRegExp(): RegExp {
Expand Down
16 changes: 6 additions & 10 deletions src/Query/Filter/PriorityField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,18 @@ export class PriorityField extends Field {

public grouper(): GrouperFunction {
return (task: Task) => {
let priorityName = 'ERROR';
switch (task.priority) {
case Priority.High:
priorityName = 'High';
break;
return ['High'];
case Priority.Medium:
priorityName = 'Medium';
break;
return ['Medium'];
case Priority.None:
priorityName = 'None';
break;
return ['None'];
case Priority.Low:
priorityName = 'Low';
break;
return ['Low'];
default:
return ['ERROR'];
}
return [`Priority ${task.priority}: ${priorityName}`];
};
}
}
6 changes: 5 additions & 1 deletion src/Query/Grouper.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Task } from '../Task';
import type { Comparator } from './Sorter';

/**
* A group-naming function, that takes a Task object and returns zero or more
Expand Down Expand Up @@ -36,9 +37,12 @@ export class Grouper {

public readonly reverse: boolean;

constructor(property: string, grouper: GrouperFunction, reverse: boolean) {
public readonly groupComparator: Comparator;

constructor(property: string, grouper: GrouperFunction, reverse: boolean, groupComparator: Comparator) {
this.property = property;
this.grouper = grouper;
this.reverse = reverse;
this.groupComparator = groupComparator;
}
}
14 changes: 3 additions & 11 deletions src/Query/TaskGroups.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,10 @@ export class TaskGroups {

private sortTaskGroups() {
const compareFn = (group1: TaskGroup, group2: TaskGroup) => {
// Compare two TaskGroup objects, sorting them by the group names at each grouping level.
const groupNames1 = group1.groups;
const groupNames2 = group2.groups;
// The containers are guaranteed to be identical sizes,
// they have one value for each 'group by' line in the query.
for (let i = 0; i < groupNames1.length; i++) {
// For now, we only have one sort option: sort by the names of the groups.
// In future, we will add control over the sorting of group headings,
// which will likely involve adjusting this code to sort by applying a Comparator
// to the first Task in each group.
// Compare two TaskGroup objects, sorting them by first task in each group.
for (let i = 0; i < this._groupers.length; i++) {
const grouper = this._groupers[i];
const result = groupNames1[i].localeCompare(groupNames2[i], undefined, { numeric: true });
const result = grouper.groupComparator(group1.tasks[0], group2.tasks[0]);
if (result !== 0) {
return grouper.reverse ? -result : result;
}
Expand Down
53 changes: 53 additions & 0 deletions tests/Query/Filter/DescriptionLengthGroupingField.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { DescriptionLengthGroupingfield } from '../../../src/Query/Filter/DescriptionLengthGroupingField';
import { fromLine } from '../../TestHelpers';
import { TaskGroups } from '../../../src/Query/TaskGroups';

describe('test a Field class that supports grouping without sorting', () => {
it('should create the grouper', () => {
const grouper = new DescriptionLengthGroupingfield().createNormalGrouper();
expect(grouper).toBeDefined();
});

it('should group in default (alphabetical) order', () => {
const tasks = [
fromLine({ line: '- [ ] descrip' }),
fromLine({ line: '- [ ] desc' }),
fromLine({ line: '- [ ] description' }),
fromLine({ line: '- [ ] d' }),
];
const grouper = [new DescriptionLengthGroupingfield().createNormalGrouper()];
const groups = new TaskGroups(grouper, tasks);

expect(groups.toString()).toMatchInlineSnapshot(`
"Groupers (if any):
- description length

Group names: [1]
#### [description length] 1
- [ ] d

---

Group names: [4]
#### [description length] 4
- [ ] desc

---

Group names: [7]
#### [description length] 7
- [ ] descrip

---

Group names: [11]
#### [description length] 11
- [ ] description

---

4 tasks
"
`);
});
});
56 changes: 51 additions & 5 deletions tests/Query/Filter/PriorityField.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { TaskBuilder } from '../../TestingTools/TaskBuilder';
import { testFilter } from '../../TestingTools/FilterTestHelpers';
import { PriorityField } from '../../../src/Query/Filter/PriorityField';
import { fromLine } from '../../TestHelpers';

import { TaskGroups } from '../../../src/Query/TaskGroups';
import {
expectTaskComparesAfter,
expectTaskComparesBefore,
Expand Down Expand Up @@ -163,15 +163,61 @@ describe('grouping by priority', () => {
});

it.each([
['- [ ] a ⏫', ['Priority 1: High']],
['- [ ] a 🔼', ['Priority 2: Medium']],
['- [ ] a', ['Priority 3: None']],
['- [ ] a 🔽', ['Priority 4: Low']],
['- [ ] a ⏫', ['High']],
['- [ ] a 🔼', ['Medium']],
['- [ ] a', ['None']],
['- [ ] a 🔽', ['Low']],
])('task "%s" should have groups: %s', (taskLine: string, groups: string[]) => {
// Arrange
const grouper = new PriorityField().createNormalGrouper().grouper;

// Assert
expect(grouper(fromLine({ line: taskLine }))).toEqual(groups);
});

it('should sort groups according to priority meaning', () => {
// Arrange
const tasks = [
fromLine({ line: '- [ ] a 🔽' }),
fromLine({ line: '- [ ] a ⏫' }),
fromLine({ line: '- [ ] a' }),
fromLine({ line: '- [ ] a 🔼' }),
];

const grouper = [new PriorityField().createNormalGrouper()];
const groups = new TaskGroups(grouper, tasks);

// Assert
expect(groups.toString()).toMatchInlineSnapshot(`
"Groupers (if any):
- priority

Group names: [High]
#### [priority] High
- [ ] a ⏫

---

Group names: [Medium]
#### [priority] Medium
- [ ] a 🔼

---

Group names: [None]
#### [priority] None
- [ ] a

---

Group names: [Low]
#### [priority] Low
- [ ] a 🔽

---

4 tasks
"
`);
});
});
42 changes: 42 additions & 0 deletions tests/Query/Filter/UrgencyField.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
expectTaskComparesEqual,
} from '../../CustomMatchers/CustomMatchersForSorting';
import { fromLine } from '../../TestHelpers';
import { TaskGroups } from '../../../src/Query/TaskGroups';

window.moment = moment;

Expand Down Expand Up @@ -78,6 +79,15 @@ describe('sorting by urgency', () => {
});

describe('grouping by urgency', () => {
beforeAll(() => {
jest.useFakeTimers();
jest.setSystemTime(new Date(2023, 5 - 1, 22));
});

afterAll(() => {
jest.useRealTimers();
});

it('supports grouping methods correctly', () => {
expect(new UrgencyField()).toSupportGroupingWithProperty('urgency');
});
Expand All @@ -96,4 +106,36 @@ describe('grouping by urgency', () => {
// Assert
expect(grouper(fromLine({ line: taskLine }))).toEqual(groups);
});

it('should sort groups from more urgent to less urgent', () => {
// Arrange
const tasks = [
new TaskBuilder().description('task1').dueDate('2023-05-22').build(),
new TaskBuilder().description('task2').dueDate('2023-05-21').build(),
];

const grouper = [new UrgencyField().createNormalGrouper()];
const groups = new TaskGroups(grouper, tasks);

// Assert
expect(groups.toString()).toMatchInlineSnapshot(`
"Groupers (if any):
- urgency

Group names: [11.21]
#### [urgency] 11.21
- [ ] task2 📅 2023-05-21

---

Group names: [10.75]
#### [urgency] 10.75
- [ ] task1 📅 2023-05-22

---

2 tasks
"
`);
});
});