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.

Sign up for GitHub

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

Jump to bottom

feat: Group by functions #1421

Closed
wants to merge 1 commit into from
Closed

feat: Group by functions #1421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 53 additions & 11 deletions src/Query/Group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import type { Task } from '../Task';
import { Priority } from '../Task';
import type { Grouping, GroupingProperty } from './Query';
import type { Grouping, GroupingArg, GroupingProperty } from './Query';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

import { TaskGroups } from './TaskGroups';
import { HappensDateField } from './Filter/HappensDateField';

/**
* A naming function, that takes a Task object and returns the corresponding group property name
*/
type Grouper = (task: Task) => string[];
type Grouper = (task: Task, arg?: GroupingArg) => string[];

/**
* Implementation of the 'group by' instruction.
Expand All @@ -32,9 +32,9 @@ export class Group {
* @param property
* @param task
*/
public static getGroupNamesForTask(property: GroupingProperty, task: Task): string[] {
const grouper = Group.groupers[property];
return grouper(task);
public static getGroupNamesForTask(grouping: Grouping, task: Task): string[] {
const grouper = Group.groupers[grouping.property];
return grouper(task, grouping.arg);
}

private static groupers: Record<GroupingProperty, Grouper> = {
Expand All @@ -45,6 +45,7 @@ export class Group {
folder: Group.groupByFolder,
happens: Group.groupByHappensDate,
heading: Group.groupByHeading,
fn: Group.groupByFn,
path: Group.groupByPath,
priority: Group.groupByPriority,
recurrence: Group.groupByRecurrence,
Expand All @@ -56,6 +57,16 @@ export class Group {
tags: Group.groupByTags,
};

private static root(task: Task) {
const path = task.path.replace(/\\/g, '/');
const separatorIndex = path.indexOf('/');
if (separatorIndex == -1) {
return '/';
} else {
return path.substring(0, separatorIndex + 1);
}
}

private static escapeMarkdownCharacters(filename: string) {
// https://wilsonmar.github.io/markdown-text-for-github-from-html/#special-characters
return filename.replace(/\\/g, '\\\\').replace(/_/g, '\\_');
Expand Down Expand Up @@ -151,13 +162,44 @@ export class Group {
return [Group.escapeMarkdownCharacters(filename)];
}

private static groupByRoot(task: Task): string[] {
const path = task.path.replace(/\\/g, '/');
const separatorIndex = path.indexOf('/');
if (separatorIndex == -1) {
return ['/'];
private static groupByFn(task: Task, arg?: GroupingArg): string[] {
const paramsArgs: [string, any][] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is some documentation in CONTRIBUTING.md about what to do when adding a new field to Task. It would be worth this PR including an update to that, to point people to this new location that would also be worth adding a new field to.

Copy link
Author

Choose a reason for hiding this comment

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

Another option for a v1 maybe could be to just pass task/t and some convenient extras not already in the task object

const paramsArgs: [string, any][] = [
    ['path', task.path.replace('.md', '')],
    ['root', Group.root(task)],
    ['t', task],
    ['task', task],
];

Copy link
Collaborator

@claremacrae claremacrae Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

[original comment moved]

['description', task.description],
['done', task.doneDate],
['due', task.dueDate],
['filename', task.filename],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

['happens', new HappensDateField().earliestDate(task)],
['header', task.precedingHeader],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest heading, for consistency with group by heading

['markdown', task.originalMarkdown],
['path', task.path.replace('.md', '')],
['priority', task.priority],
['recurrence', task.recurrence],
['root', Group.root(task)],
['scheduled', task.scheduledDate],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

['start', task.startDate],
['status', task.status],
['t', task],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this expose the Task object?

Copy link
Author

Choose a reason for hiding this comment

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

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'

Copy link
Collaborator

Choose a reason for hiding this comment

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

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...)

Copy link
Author

@weirdhorror weirdhorror Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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],
];

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

['tags', task.tags],
['task', task],
['urgency', task.urgency],
];

const params = paramsArgs.map(([p]) => p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const groupBy = arg && new Function(...params, `return ${arg}`);

if (groupBy instanceof Function) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

const args = paramsArgs.map(([_, a]) => a);
const result = groupBy(...args);
const group = typeof result === 'string' ? result : 'Error with group result';

return [Group.escapeMarkdownCharacters(group)];
} else {
return ['Error parsing group function'];
}
return [Group.escapeMarkdownCharacters(path.substring(0, separatorIndex + 1))];
}

private static groupByRoot(task: Task): string[] {
return [Group.escapeMarkdownCharacters(Group.root(task))];
}

private static groupByBacklink(task: Task): string[] {
Expand Down
2 changes: 1 addition & 1 deletion src/Query/IntermediateTaskGroups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export class IntermediateTaskGroups {
const nextTreeLevel = [];
for (const currentTreeNode of currentTreeLevel) {
for (const task of currentTreeNode.values) {
const groupNames = Group.getGroupNamesForTask(grouping.property, task);
const groupNames = Group.getGroupNamesForTask(grouping, task);
for (const groupName of groupNames) {
let child = currentTreeNode.children.get(groupName);
if (child === undefined) {
Expand Down
29 changes: 24 additions & 5 deletions src/Query/Query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type GroupingProperty =
| 'folder'
| 'happens'
| 'heading'
| 'fn'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

| 'path'
| 'priority'
| 'recurrence'
Expand All @@ -26,7 +27,13 @@ export type GroupingProperty =
| 'start'
| 'status'
| 'tags';
export type Grouping = { property: GroupingProperty };

export type GroupingArg = string | null;

export type Grouping = {
Comment on lines +31 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

property: GroupingProperty;
arg?: GroupingArg;
};

export class Query implements IQuery {
public source: string;
Expand All @@ -39,7 +46,7 @@ export class Query implements IQuery {
private _grouping: Grouping[] = [];

private readonly groupByRegexp =
/^group by (backlink|done|due|filename|folder|happens|heading|path|priority|recurrence|recurring|root|scheduled|start|status|tags)/;
/^group by (backlink|done|due|filename|fn|folder|happens|heading|path|priority|recurrence|recurring|root|scheduled|start|status|tags)[\s]*(.*)/;

private readonly hideOptionsRegexp =
/^(hide|show) (task count|backlink|priority|start date|scheduled date|done date|due date|recurrence rule|edit button|urgency)/;
Expand Down Expand Up @@ -227,10 +234,22 @@ export class Query implements IQuery {

private parseGroupBy({ line }: { line: string }): void {
const fieldMatch = line.match(this.groupByRegexp);

if (fieldMatch !== null) {
this._grouping.push({
property: fieldMatch[1] as GroupingProperty,
});
const property = fieldMatch[1] as GroupingProperty;

if (property !== 'fn') {
this._grouping.push({
property: property,
});
} else if (fieldMatch[2] !== null) {
this._grouping.push({
property: property,
arg: fieldMatch[2] as GroupingArg,
});
} else {
this._error = 'do not understand fn query grouping';
}
} else {
this._error = 'do not understand query grouping';
}
Expand Down
3 changes: 2 additions & 1 deletion tests/Group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import { fromLine } from './TestHelpers';
window.moment = moment;

function checkGroupNamesOfTask(task: Task, property: GroupingProperty, expectedGroupNames: string[]) {
const group = Group.getGroupNamesForTask(property, task);
const grouping: Grouping = { property };
const group = Group.getGroupNamesForTask(grouping, task);
Comment on lines +13 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

expect(group).toEqual(expectedGroupNames);
}

Expand Down