feat: Group by functions

[weirdhorror]

Description

Proposal to add a group by fn feature, to group tasks by the return value of JavaScript expressions.

not done
group by fn root === "journal/" ? root : path

In this example the value of root and path are available in the expression body via named arguments passed to the function.

Motivation and Context

My initial motivation was simply the example above, to group tasks from my daily log notes under a single "journal" heading, otherwise group all other tasks by the full note path.

How has this been tested?

This is just a proposal. Happy to add tests if accepted.

Screenshots (if appropriate)

Types of changes

Changes visible to users:

• Bug fix (prefix: fix - non-breaking change which fixes an issue)
• New feature (prefix: feat - non-breaking change which adds functionality)
• Breaking change (prefix: feat!! or fix!! - fix or feature that would cause existing functionality to not work as expected)
• Documentation (prefix: docs - improvements to any documentation content)
• Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)

Internal changes:

• Refactor (prefix: refactor - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
• Tests (prefix: test - additions and improvements to unit tests and the smoke tests)
• Infrastructure (prefix: chore - examples include GitHub Actions, issue templates)

Checklist

• 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 accordingly.
• My change has adequate Unit Test coverage.

Terms

• My contribution follow this project's contributing guide
• I agree to follow this project's Code of Conduct

[claremacrae]

Hi, thanks ever so much for offering to contribute to Tasks.

What does your sample function group by line do exactly please, and what problem is this solving for users?

[claremacrae]

Apologies, I see the explanation now:

My initial motivation was simply the example above, to group tasks from my daily log notes under a single "journal" heading, otherwise group all other tasks by the full note path.

[claremacrae]

@weirdhorror Wow. I have to be honest and say that it scares me a little! 🤣 Like, is any sanitising of the code needed, for example? Are there going to be issues with parsing if mismatched quotes and things like that?

BUT I can see the power of it, and I do think it would be worth working up to release.

FYI The features I was planning to implement soon for group by are:

• Allowing users to supply custom date formats for group-based dates (this is highly requested)
• Allowing reversed sorting of the group headings - such as group by done reverse (also highly requested)

Can you see how your changes to the regular expression for parsing would cope with the above?

A lot of Tasks users will be non-developers, so things like the ternary operator and using === will all need to be well documented. I would be very happy to do final editing of docs, but would ask you to at least write a first draft of it, please. Would you feel up to that?

[claremacrae]

I've not yet had time to run the code, but here are some thoughts from reading the code through several times.

What I can't tell from this is what users are going to have to do if grouping by dates for example... Are there going to be lots of formatting and YYYY-MM-DD in user code?

[claremacrae]

I would prefer function rather than fn, to make it less cryptic to non-developers...

[claremacrae]

Picking an arbitrary place to add a comment about a missing test...

CONTRIBUTING has a section on tests to add for new instructions: there is a place in Query.test.ts to add any new sort instructions, to ensure that they are correctly hooked up and parsed, please.

[claremacrae]

Maybe worth adding folder, for consistency with group by folder?

[claremacrae]

I suggest heading, for consistency with group by heading

[claremacrae]

Will this expose the Task object?

[weirdhorror]

Yes, as a variable t in the group by fn body. I also include it as a task var but it seemed useful to have a shorter name too

group by fn t.status ===  'done' : 'Done' : 'Todo'

[claremacrae]

I ask because I care a lot about avoiding breaking changes for users - not requiring them to rewrite tasks blocks if at all possible.

In #1249 I tried to explore whether making Task objects visible to users - with all its public data - would tie my hands and prevent me from doing future refactorings.

For that reason I started implementing an abstraction for dates, but didn't get very far as a lot of other stuff has cropped up.

I am therefore a little nervous about this PR's exposing of raw dates in this PR, and what restrictions it would mean on future improvements to code maintainability.

And considerably more nervous at the idea of exposing the entire Task class.

I am prepared to be persuaded away from that view, if someone can show me ways of exposing the inner details now, and not hampering future development... I just personally can't see a way right now.

The background to all this is that there are a load of missing abstractions that make addition of new features and comprehension of the code harder than I would like. And so I am trying to juggle improving the design with user support and PRs... (It's a nice position to be in, of course...)

[weirdhorror]

Oh I see, that all makes total sense. Thanks for explaining that to me.

Hmm, what if instead of my suggestion here #1421 (comment) then, we went kinda the opposite direction and only exposed string/number/primitive values, no task/t, dates or complex objects.

const paramsArgs: [string, any][] = [
    ['description', task.description],
    ['done', task.doneDate?.format('YYYY-MM-DD')],
    ['due', task.dueDate?.format('YYYY-MM-DD')],
    ['filename', task.filename],
    ['happens', new HappensDateField().earliestDate(task)?.format('YYYY-MM-DD')],
    ['heading', task.precedingHeader],
    ['markdown', task.originalMarkdown],
    ['path', task.path.replace('.md', '')],
    ['priority', task.priority],
    ['root', Group.root(task)],
    ['scheduled', task.scheduledDate?.format('YYYY-MM-DD')],
    ['start', task.startDate?.format('YYYY-MM-DD')],
    ['status', task.status.toLowerCase()],
    ['tags', task.tags],
    ['urgency', task.urgency],
];

[claremacrae]

Thank you for being so understanding.

Your reply game me another idea. Refactoring the whole of tasks to use a new date class will take a while.

For info, I just pushed refactor-add-TaskDate-class branch which is 2 months old and far, far for complete.

After reading you reply, I started to wonder - what if the dates in this PR were exposed as TaskDate objects, since that is the direction the code will be going in at some point.

I would like someone familiar with idiomatic TypeScript (I'm not) to review TaskDate and say if anything needs to be improved, but the class on its own could be made suitable for release in not very long. (I know it needs JSDocs).

So the diff from your above would look something like:

+ ['start', task.startDate?.format('YYYY-MM-DD')]
- ['start', new TaskDate(task.startDate)]

We could add methods to TaskDate based on what we learn from your work on this PR...

Does that make sense? What do you think?

[weirdhorror]

This makes total sense, yeah. FWIW I don't have much preference for which fields are exposed or in which format, etc. as long as I can (selfishly) still solve my original little problem lol. 😇

Funny, I was even tempted to write a little date helper for my example (instead of repeating ?.format('YYYY-MM-DD')) but wanted to stay focused.

Wish I could be of more help re: idiomatic TypeScript and TaskDate, but I've only recently started using TypeScript myself. 🖤

But please let me know what else I can do to help with this PR anytime.

[claremacrae]

But please let me know what else I can do to help with this PR anytime.

I think the simplest thing is for you to continue with the plugin basically as is, just responding the code review feedback.
If you leave it returning task.start etc, then I can add the TaskDate class and hook it up after this PR is merged.
That way, you could focus on what you have already done, which is such a massive step forward.

[claremacrae]

Please could tests include a use of each of those fields in the group, to show how they are rendered?
I am interested in seeing how dates are rendered, for example.

[claremacrae]

I am soon going to be refactoring the grouping code to remove GroupingProperty.

I suggest we work on getting this PR merged first, to avoid this otherwise being blocked on a non-trivial refactoring.

[claremacrae]

A comment here to explain what's going on would be appreciated.

[claremacrae]

How might it not be an instance of function at this point?

[weirdhorror]

I think if arg? is nullish const groupBy = arg && new Function.. would make groupBy nullish as well

[claremacrae]

Some JSDocs explaining the purpose of these two types would be good.

[claremacrae]

I had no idea that something this flexible could be implemented so succinctly!

[weirdhorror]

Hi, thanks ever so much for offering to contribute to Tasks.

What does your sample function group by line do exactly please

In my vault I have my daily notes set to a folder named "journal" and I often add one-off tasks in daily notes.

Using group by path to list my tasks creates separate groups for every daily note that has a task or three in it, which is visually distracting and journal/2022-12-22, etc, etc. are not meaningfully group names for those one-off tasks.

Before

journal/2022-12-20
  [ ] task foo from note journal/2022-12-20

journal/2022-12-15
  [ ] task bar from journal/2022-12-15

journal/2022-12-10
  [ ] task baz from journal/2022-12-10

journal/...
journal/...
journal/...

projects/foo
  [ ] task one from projects/foo
  [ ] task two from projects/foo

topics/research-buzz
  [ ] task one from topics/research-buzz
  [ ] task two from topics/research-buzz

So in this example:

not done
group by fn root === "journal/" ? root : path

All tasks in any note with a root path === "journal/" will be grouped under "journal/" otherwise the task will be grouped by the full path of the note.

After

journal/
  [ ] task foo from note journal/2022-12-20
  [ ] task bar from journal/2022-12-15
  [ ] task baz from journal/2022-12-10
  [ ] etc

projects/foo
  [ ] task one from projects/foo
  [ ] task two from projects/foo

topics/research-buzz
  [ ] task one from topics/research-buzz
  [ ] task two from topics/research-buzz

and what problem is this solving for users?

This group by fn feature could be used as an escape hatch to create all kinds of custom group functions that don't exist out of the box.

I should probably try to think of some good, useful examples but here are two off the top of my head:

group by fn description.startsWith('Read') ? 'Readme' : path

group by fn priority > 8 ? 'now' : 'later'

[weirdhorror]

@weirdhorror Wow. I have to be honest and say that it scares me a little! 🤣

Hah, yes, it's a bit wild I know. I think using new Function and passing the named args in is a bit less #yolo than eval which generally scares me 👀

Like, is any sanitising of the code needed, for example? Are there going to be issues with parsing if mismatched quotes and things like that?

Anything to the right of group by fn will be evaluated as a JavaScript function, so if the function call fails currently it just groups everything by Error with group result or Error parsing group function. Not sure if this is ideal or if failure should bubble up to a full parse error, like Tasks query: do not understand query ...?

FYI The features I was planning to implement soon for group by are:

• Allowing users to supply custom date formats for group-based dates (this is highly requested)
• Allowing reversed sorting of the group headings - such as group by done reverse (also highly requested)

Can you see how your changes to the regular expression for parsing would cope with the above?

Can you share the proposed syntax for custom date formats?

I think group by <field> reverse could be done via the GroupingArg. You can see here I'm only passing arg in the case of group by fn, but this could be changed to pass extra args to any new grouping functions that accept them.

A lot of Tasks users will be non-developers, so things like the ternary operator and using === will all need to be well documented. I would be very happy to do final editing of docs, but would ask you to at least write a first draft of it, please. Would you feel up to that?

Yes of course! Note, the group by fn currently would run any JavaScript, so the full scope of what's possible is ... the entire language, haha, but I think it would be great to have some good examples showing non-developers how to use this for common customizations.

[claremacrae]

Error handling

Like, is any sanitising of the code needed, for example? Are there going to be issues with parsing if mismatched quotes and things like that?

Anything to the right of group by fn will be evaluated as a JavaScript function, so if the function call fails currently it just groups everything by Error with group result or Error parsing group function. Not sure if this is ideal or if failure should bubble up to a full parse error, like Tasks query: do not understand query ...?

Oooh - that's a really good point.

Some of the people using this feature will have minimal to no JS experience, so the more info we can give them about any errors, and the earlier the error can happen, the better.

So I think the failures should indeed bubble up, please, as you put so well...

Potential new group by features

FYI The features I was planning to implement soon for group by are:

• Allowing users to supply custom date formats for group-based dates (this is highly requested)
• Allowing reversed sorting of the group headings - such as group by done reverse (also highly requested)

Can you see how your changes to the regular expression for parsing would cope with the above?

Can you share the proposed syntax for custom date formats?

Well I'm not sure, but maybe something like:

group by due [[YYYY/MM/YYYY-MM-DD dddd]]

Which in my main vault would would like to the daily note for the particular day.

That's just a suggestion.

Aside: How sort by instructions now work

I think group by <field> reverse could be done via the GroupingArg. You can see here I'm only passing arg in the case of group by fn, but this could be changed to pass extra args to any new grouping functions that accept them.

Now might be a good time to point you to the refactoring I've been doing recently on sort by.

Here's the new code added to Field.ts:

obsidian-tasks/src/Query/Filter/Field.ts

Lines 102 to 233 in fab4778

	// -----------------------------------------------------------------------------------------------------------------
	// Sorting
	// -----------------------------------------------------------------------------------------------------------------
	/**
	* Return whether the code for this field implements sorting of tasks
	*/
	public supportsSorting(): boolean {
	// TODO Make abstract
	return false;
	}
	/**
	* Parse a 'sort by' line and return a Sorting 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 {
	if (!this.supportsSorting()) {
	return null;
	}
	if (!this.canCreateSorterForLine(line)) {
	return null;
	}
	const sorting = this.createSorterFromLine(line);
	if (sorting) {
	return sorting;
	}
	return null;
	}
	/**
	* 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 Sorting} object or null.
	*
	* This default implementation works for all fields that support
	* the default sorting pattern of `sort by <fieldName> (reverse)?`.
	*
	* Fields that offer more complicated 'sort by' options can override
	* this method.
	*
	* @param line - A 'sort by' line from a ```tasks``` block.
	*
	* @see {@link canCreateSorterForLine}
	*/
	public createSorterFromLine(line: string): Sorting | null {
	if (!this.supportsSorting()) {
	return null;
	}
	const match = Field.getMatch(this.sorterRegExp(), line);
	if (match === null) {
	return null;
	}
	const reverse = !!match[1];
	return this.createSorter(reverse);
	}
	/**
	* Return a regular expression that will match a correctly-formed
	* instruction line for sorting Tasks by this field.
	*
	* Throws if this field does not support sorting.
	*
	* `match[1]` will be either `reverse` or undefined.
	*
	* Fields that offer more complicated 'sort by' options can override
	* this method.
	*/
	protected sorterRegExp(): RegExp {
	if (!this.supportsSorting()) {
	throw Error(`sorterRegExp() unimplemented for ${this.fieldNameSingular()}`);
	}
	return new RegExp(`^sort by ${this.fieldNameSingular()}( reverse)?`);
	}
	/**
	* Return a function to compare two Task objects, for use in sorting by this field's value.
	*/
	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.
	* @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());
	}
	/**
	* Create a {@link Sorting} object for sorting tasks by this field's value,
	* in the standard/normal sort order for this field.
	*
	* @see {@link createReverseSorter}
	*/
	public createNormalSorter(): Sorting {
	return this.createSorter(false);
	}
	/**
	* Create a {@link Sorting} 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 {
	return this.createSorter(true);
	}

Fields that use the standard sort pattern (sort by filename [reverse]) only need to implement two methods:

obsidian-tasks/src/Query/Filter/PriorityField.ts

Lines 73 to 81 in fab4778

	public supportsSorting(): boolean {
	return true;
	}
	public comparator(): Comparator {
	return (a: Task, b: Task) => {
	return a.priority.localeCompare(b.priority);
	};
	}

But fields that support additional sort options - like sort by tag 3 - can do a little more, protecting all the others from needing to know about the corner cases:

obsidian-tasks/src/Query/Filter/TagsField.ts

Lines 24 to 98 in fab4778

	// -----------------------------------------------------------------------------------------------------------------
	// Sorting
	// -----------------------------------------------------------------------------------------------------------------
	public supportsSorting(): boolean {
	return true;
	}
	/** Overridden to add support for tag number.
	*
	* @param line
	*/
	public createSorterFromLine(line: string): Sorting | null {
	const match = line.match(this.sorterRegExp());
	if (match === null) {
	return null;
	}
	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 a regular expression that will match a correctly-formed
	* instruction line for sorting Tasks by tag.
	*
	* `match[1]` will be either `reverse` or undefined.
	* `match[2]` will be either the tag number or undefined.
	*/
	protected sorterRegExp(): RegExp {
	return /^sort by tag( reverse)?[\s]*(\d+)?/;
	}
	/**
	* Create a ${@link Comparator} that sorts by the first tag.
	*/
	public comparator(): Comparator {
	return TagsField.makeCompareByTagComparator(1);
	}
	private static makeCompareByTagComparator(propertyInstance: number): Comparator {
	return (a: Task, b: Task) => {
	// If no tags then assume they are equal.
	if (a.tags.length === 0 && b.tags.length === 0) {
	return 0;
	} else if (a.tags.length === 0) {
	// a is less than b
	return 1;
	} else if (b.tags.length === 0) {
	// b is less than a
	return -1;
	}
	// Arrays start at 0 but the users specify a tag starting at 1.
	const tagInstanceToSortBy = propertyInstance - 1;
	if (a.tags.length < propertyInstance && b.tags.length >= propertyInstance) {
	return 1;
	} else if (b.tags.length < propertyInstance && a.tags.length >= propertyInstance) {
	return -1;
	} else if (a.tags.length < propertyInstance && b.tags.length < propertyInstance) {
	return 0;
	}
	if (a.tags[tagInstanceToSortBy] < b.tags[tagInstanceToSortBy]) {
	return -1;
	} else if (a.tags[tagInstanceToSortBy] > b.tags[tagInstanceToSortBy]) {
	return 1;
	} else {
	return 0;
	}
	};
	}

Dividing up the group by code a lot more quickly

The above is a culmination of a series of careful refactorings. From what I learned, I would be able to divide up the group by code a lot more quickly.

When I first read this PR, I initially toyed with the idea of suggesting that it be picked up after I had divided up the group by code, as the new implementation would be a lot simpler, since all the new code would be in one file - which would have its own tests. And could have its own regex.

And over time, it could be taught to do sort by function and possible even function-based filters.

Docs

A lot of Tasks users will be non-developers, so things like the ternary operator and using === will all need to be well documented. I would be very happy to do final editing of docs, but would ask you to at least write a first draft of it, please. Would you feel up to that?

Yes of course! Note, the group by fn currently would run any JavaScript, so the full scope of what's possible is ... the entire language, haha, but I think it would be great to have some good examples showing non-developers how to use this for common customizations.

Awesome - thank you!

[weirdhorror]

Error handling

So I think the failures should indeed bubble up, please, as you put so well...

OK, let me know if you'd like me to tinker with bubbling up errors from Group.by, or if I should hold off for a bit. My hunch is that this might be a bigger change, right, since currently Group.by always returns TaskGroups but I think would need to be changed to return a Result/Either type.

When I first read this PR, I initially toyed with the idea of suggesting that it be picked up after I had divided up the group by code, as the new implementation would be a lot simpler, since all the new code would be in one file - which would have its own tests. And could have its own regex.

And over time, it could be taught to do sort by function and possible even function-based filters.

Oh, yes, and let me just say too, I promise I won't be hurt if you decide to ice this PR for time being, or even if you decide it simply doesn't fit into this plugin at all rn. Whatever you think is right.

Also thanks for all the explanations and links, etc. I'm going to read them more in depth now.

[claremacrae]

OK, let me know if you'd like me to tinker with bubbling up errors from Group.by, or if I should hold off for a bit. My hunch is that this might be a bigger change, right, since currently Group.by always returns TaskGroups but I think would need to be changed to return a Result/Either type.

I do think your hunch is right, and let's stick with the design you currently have.

(I was briefly going to suggest that the error-checking be at the point when the group by function line is parsed, as that is the point at which a syntax-type error would need to be reported. Like maybe Query.parseGroupBy() could be persuaded to create a Task list with one dummy task, and group it. But looking at the code, I think it would just be too complicated - and so let's stick with the error-checking at the point of grouping for now.)

When I first read this PR, I initially toyed with the idea of suggesting that it be picked up after I had divided up the group by code, as the new implementation would be a lot simpler, since all the new code would be in one file - which would have its own tests. And could have its own regex.
And over time, it could be taught to do sort by function and possible even function-based filters.

Oh, yes, and let me just say too, I promise I won't be hurt if you decide to ice this PR for time being, or even if you decide it simply doesn't fit into this plugin at all rn. Whatever you think is right.

Gosh, thank you very much indeed.

I definitely think it fits. I also definitely think it's worth completing - and I think it will open up some other ideas later on.

So please do continue with the current PR!

[claremacrae]

#1133 looks like a request for the exact same facility.

[weirdhorror]

I definitely think it fits. I also definitely think it's worth completing - and I think it will open up some other ideas later on.

So please do continue with the current PR!

Got pulled into a holiday whirlwind, but planning to resume this next week fyi 🙃

[claremacrae]

Hi @weirdhorror, Happy New Year!

I've got someone offering to pair with me on some groups-related refactoring - and so was wondering how this was going...

If you thought this might be coming soon-ish (for any definition of soon!) then I would hold off and wait for this... to avoid creating merge conflicts...

However, the merge conflicts will be fairly easy to fix, so there is no absolutely no pressure for you to work on this, I promise - me and the other person could always work on the refactoring anyways, in parallel or advance of this, if that was better for you.

[claremacrae]

I'm picking this up and porting it to the latest code. Very excited - thank you @weirdhorror! 😄