refactor: move default comparator to Grouper #2018
Conversation
Which of the steps listed in this comment in #1966 is this? I'm just conscious I rushed you to a partial fix of step 1, and so it is incomplete....
|
| this.property = property; | ||
| this.grouper = grouper; | ||
| this.reverse = reverse; | ||
| this.comparator = (a: Task, b: Task) => { | ||
| const result = comparator(a, b); |
There was a problem hiding this comment.
Maybe I'm missing something but we talked elsewhere about comparator() throwing if sorting is unimplemented in the field...
There was a problem hiding this comment.
True, but this is just refactoring - moving the comparator from TaskGroups.sortTaskGroups into Field.defaultComparator. For now Field.comparator() is never called, so no throwing when sorting is not supported
There was a problem hiding this comment.
Ah - right. Now I see what's going on.
I need to actually test this out and think about it some more.
I would be a lot more comfortable, and the review would be a lot quicker, if there were already the thorough group-sorting tests that we have talked about previously.
There was a problem hiding this comment.
If I recall correctly, you said we don't need to have extensive tests till the end to start changing the code. I may have misunderstood.
There was a problem hiding this comment.
I can quite believe I said that, but it wasn't my intention.
Once it became possible to change the sort order of groups, it revealed a gaping hole in the grouping tests, which had previously only tested the names of groups and not the order.
I would like that hole to be fixed for all fields before the grouping code is changed please.
|
I am not sure that this is a refactor, but I think we don't yet have good enough test coverage to be able to prove that anyway.... (Caveat: I have only read the code and not run it...) |
tests/Query/Filter/TagsField.test.ts
Outdated
| @@ -673,4 +673,12 @@ describe('grouping by tag', () => { | |||
| " | |||
| `); | |||
| }); | |||
|
|
|||
| it('should sort groups for TaskField', () => { | |||
There was a problem hiding this comment.
I think this should be TagsField
There was a problem hiding this comment.
oops, fixed, thanks
tests/Query/Filter/TagsField.test.ts
Outdated
| const taskLines = ['- [ ] a #tag1', '- [ ] a #tag2', '- [ ] a #tag1 #tag2', '- [ ] a']; | ||
| const tasks = taskLines.map((taskLine) => fromLine({ line: taskLine })); | ||
|
|
||
| expect({ grouper, tasks }).groupHeadingsToBe(['(No tags)', '#tag1', '#tag2']); | ||
| }); |
There was a problem hiding this comment.
If we stick with the idea of having a helper function that creates a selection of tasks with a wide range of values, I think it would be good for this to also include some nested tags to see how they sort...
There was a problem hiding this comment.
Ah right! I had a feeling that I need to add more to this test! Thanks =)
Done.
This is making the change is easy for the PR2 |
src/Query/Filter/Field.ts
Outdated
| const groupA = this.grouper()(a)[0]; | ||
| const groupB = this.grouper()(b)[0]; |
There was a problem hiding this comment.
What happens when you test this on a field that does not support grouping?
There was a problem hiding this comment.
Like description, for example. But better to create a fake field in the test that is guaranteed to not support grouping.
|
We should probably chat about this next time we pair - it will be a lot more efficient - sorry for not thinking about that earlier! 😄 |
ilandikov
force-pushed
the
refactor-move-default-comparator-to-grouper
branch
2 times, most recently
from
ddda591
to
489241d
Compare
|
@claremacrae I have re-pushed this branch after the tests have been done in #2024 I'm still disturbed by the fact that group order test in This probably shall be coming from that change of the comparison in const result = groupNames1[i].localeCompare(groupNames2[i], undefined, { numeric: true });and now it is done between the first tasks of each group: const result = grouper.comparator(group1.tasks[0], group2.tasks[0]);On the other hand, the comparison shall be finally done between two tasks since export type Comparator = (a: Task, b: Task) => number;If you have a quick clue for me - I'm buying. Otherwise let's discuss this during our next pairing session =) |
|
I'm sorry I don't have time to look at this right now and for a little while. I am desperate to finish off a big new feature I've been working on for a couple of weeks. |
ilandikov
force-pushed
the
refactor-move-default-comparator-to-grouper
branch
from
489241d
to
25501ad
Compare
|
Closing this PR as we just discussed "local" fixes for |
Description
Move the default comparator from
TaskGroupstoGrouper.Motivation and Context
Preparing for fixes group ordering of
UrgencyFieldand others. Next step will be the fix.Split of #1966
How has this been tested?
Unit test, some new test added.
Screenshots (if appropriate)
Types of changes
Internal changes:
refactor- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)Checklist
yarn run lint.Terms