From 1c5c6dfc136738603be037dfa93c26204904af4f Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Thu, 26 Jan 2023 22:51:50 +0000 Subject: [PATCH 1/3] refactor: Remove cyclic dependency due to DateField.compareByDate() Moved it and its tests to new file DateTools.ts This removes these cyclic dependencies: 4) Task.ts > Query/Filter/DateField.ts > Query/Filter/Field.ts > Query/Filter/Filter.ts 5) Task.ts > Query/Filter/DateField.ts > Query/Filter/Field.ts > Query/Grouper.ts 6) Task.ts > Query/Filter/DateField.ts > Query/Filter/Field.ts > Query/Sorter.ts 7) Task.ts > Query/Filter/DateField.ts See #1591 --- src/Query/Filter/DateField.ts | 27 ++----------------- src/Query/Filter/HappensDateField.ts | 5 ++-- src/Recurrence.ts | 8 +++--- src/Task.ts | 4 +-- src/lib/DateTools.ts | 23 ++++++++++++++++ .../CustomMatchersForSorting.ts | 4 +-- .../DateTools.test.ts} | 2 +- 7 files changed, 37 insertions(+), 36 deletions(-) create mode 100644 src/lib/DateTools.ts rename tests/{Query/Filter/DateField.test.ts => lib/DateTools.test.ts} (94%) diff --git a/src/Query/Filter/DateField.ts b/src/Query/Filter/DateField.ts index 68881d9fcb..1f68cbda0a 100644 --- a/src/Query/Filter/DateField.ts +++ b/src/Query/Filter/DateField.ts @@ -3,6 +3,7 @@ import type { Task } from '../../Task'; import { DateParser } from '../DateParser'; import { Explanation } from '../Explain/Explanation'; import type { Comparator } from '../Sorter'; +import { compareByDate } from '../../lib/DateTools'; import { Field } from './Field'; import { Filter, FilterOrErrorMessage } from './Filter'; import { FilterInstructions } from './FilterInstructions'; @@ -127,31 +128,7 @@ export abstract class DateField extends Field { public comparator(): Comparator { return (a: Task, b: Task) => { - return DateField.compareByDate(this.date(a), this.date(b)); + return compareByDate(this.date(a), this.date(b)); }; } - - public static compareByDate(a: moment.Moment | null, b: moment.Moment | null): -1 | 0 | 1 { - if (a !== null && b === null) { - return -1; - } else if (a === null && b !== null) { - return 1; - } else if (a !== null && b !== null) { - if (a.isValid() && !b.isValid()) { - return -1; - } else if (!a.isValid() && b.isValid()) { - return 1; - } - - if (a.isAfter(b)) { - return 1; - } else if (a.isBefore(b)) { - return -1; - } else { - return 0; - } - } else { - return 0; - } - } } diff --git a/src/Query/Filter/HappensDateField.ts b/src/Query/Filter/HappensDateField.ts index bf9cf187cb..88562a5161 100644 --- a/src/Query/Filter/HappensDateField.ts +++ b/src/Query/Filter/HappensDateField.ts @@ -3,6 +3,7 @@ import type { Task } from '../../Task'; import { DateParser } from '../DateParser'; import { Explanation } from '../Explain/Explanation'; import type { Comparator } from '../Sorter'; +import { compareByDate } from '../../lib/DateTools'; import { Field } from './Field'; import { Filter, FilterOrErrorMessage } from './Filter'; import { FilterInstructions } from './FilterInstructions'; @@ -92,7 +93,7 @@ export class HappensDateField extends Field { */ public earliestDate(task: Task): Moment | null { const happensDates = new HappensDateField().dates(task); - const sortedHappensDates = happensDates.sort(DateField.compareByDate); + const sortedHappensDates = happensDates.sort(compareByDate); return sortedHappensDates[0]; } @@ -120,7 +121,7 @@ export class HappensDateField extends Field { */ public comparator(): Comparator { return (a: Task, b: Task) => { - return DateField.compareByDate(this.earliestDate(a), this.earliestDate(b)); + return compareByDate(this.earliestDate(a), this.earliestDate(b)); }; } } diff --git a/src/Recurrence.ts b/src/Recurrence.ts index 8d8d91d7ee..af001e65f1 100644 --- a/src/Recurrence.ts +++ b/src/Recurrence.ts @@ -1,6 +1,6 @@ import type { Moment } from 'moment'; import { RRule } from 'rrule'; -import { DateField } from './Query/Filter/DateField'; +import { compareByDate } from './lib/DateTools'; export class Recurrence { private readonly rrule: RRule; @@ -195,13 +195,13 @@ export class Recurrence { } // Compare Date fields - if (DateField.compareByDate(this.startDate, other.startDate) !== 0) { + if (compareByDate(this.startDate, other.startDate) !== 0) { return false; } - if (DateField.compareByDate(this.scheduledDate, other.scheduledDate) !== 0) { + if (compareByDate(this.scheduledDate, other.scheduledDate) !== 0) { return false; } - if (DateField.compareByDate(this.dueDate, other.dueDate) !== 0) { + if (compareByDate(this.dueDate, other.dueDate) !== 0) { return false; } diff --git a/src/Task.ts b/src/Task.ts index cc54240502..0aa65d3b04 100644 --- a/src/Task.ts +++ b/src/Task.ts @@ -6,11 +6,11 @@ import { getSettings } from './Config/Settings'; import { StatusRegistry } from './StatusRegistry'; import type { Status } from './Status'; import { Urgency } from './Urgency'; -import { DateField } from './Query/Filter/DateField'; import { renderTaskLine } from './TaskLineRenderer'; import type { TaskLineRenderDetails } from './TaskLineRenderer'; import { DateFallback } from './DateFallback'; import * as RegExpTools from './lib/RegExpTools'; +import { compareByDate } from './lib/DateTools'; /** * When sorting, make sure low always comes after none. This way any tasks with low will be below any exiting @@ -686,7 +686,7 @@ export class Task { for (const el of args) { const date1 = this[el] as Moment | null; const date2 = other[el] as Moment | null; - if (DateField.compareByDate(date1, date2) !== 0) { + if (compareByDate(date1, date2) !== 0) { return false; } } diff --git a/src/lib/DateTools.ts b/src/lib/DateTools.ts new file mode 100644 index 0000000000..f6581527f9 --- /dev/null +++ b/src/lib/DateTools.ts @@ -0,0 +1,23 @@ +export function compareByDate(a: moment.Moment | null, b: moment.Moment | null): -1 | 0 | 1 { + if (a !== null && b === null) { + return -1; + } else if (a === null && b !== null) { + return 1; + } else if (a !== null && b !== null) { + if (a.isValid() && !b.isValid()) { + return -1; + } else if (!a.isValid() && b.isValid()) { + return 1; + } + + if (a.isAfter(b)) { + return 1; + } else if (a.isBefore(b)) { + return -1; + } else { + return 0; + } + } else { + return 0; + } +} diff --git a/tests/CustomMatchers/CustomMatchersForSorting.ts b/tests/CustomMatchers/CustomMatchersForSorting.ts index f5a18b8b0a..32a2d6b342 100644 --- a/tests/CustomMatchers/CustomMatchersForSorting.ts +++ b/tests/CustomMatchers/CustomMatchersForSorting.ts @@ -1,8 +1,8 @@ import type moment from 'moment'; import { DateParser } from '../../src/Query/DateParser'; -import { DateField } from '../../src/Query/Filter/DateField'; import type { Sorter } from '../../src/Query/Sorter'; import type { Task } from '../../src/Task'; +import { compareByDate } from '../../src/lib/DateTools'; declare global { namespace jest { @@ -26,7 +26,7 @@ expect.extend({ let b: moment.Moment | null = null; if (dateB !== null) b = DateParser.parseDate(dateB); - const actual = DateField.compareByDate(a, b); + const actual = compareByDate(a, b); const pass = actual === expected; const message = () => `${dateA} < ${dateB}: expected=${expected} actual=${actual}`; diff --git a/tests/Query/Filter/DateField.test.ts b/tests/lib/DateTools.test.ts similarity index 94% rename from tests/Query/Filter/DateField.test.ts rename to tests/lib/DateTools.test.ts index af4524a196..f9f6d7fc34 100644 --- a/tests/Query/Filter/DateField.test.ts +++ b/tests/lib/DateTools.test.ts @@ -9,7 +9,7 @@ import { expectDateComparesAfter, expectDateComparesBefore, expectDateComparesEqual, -} from '../../CustomMatchers/CustomMatchersForSorting'; +} from '../CustomMatchers/CustomMatchersForSorting'; // These are lower-level tests that the Task-based ones above, for ease of test coverage. describe('compareBy', () => { From 8bb1b7ac982804a589ed397873d8cb16fd96f0dd Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Thu, 26 Jan 2023 23:06:56 +0000 Subject: [PATCH 2/3] refactor: Remove dependency Config/CustomStatusModal.ts > main.ts This in turn removes the cyclic dependency: Config/CustomStatusModal.ts > main.ts > Config/SettingsTab.ts See #1591 --- src/Config/CustomStatusModal.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Config/CustomStatusModal.ts b/src/Config/CustomStatusModal.ts index 806d33e505..696ee5be86 100644 --- a/src/Config/CustomStatusModal.ts +++ b/src/Config/CustomStatusModal.ts @@ -1,6 +1,6 @@ import { Modal, Notice, Setting, TextComponent } from 'obsidian'; +import type { Plugin } from 'obsidian'; import { StatusConfiguration, StatusType } from '../StatusConfiguration'; -import type TasksPlugin from '../main'; import { StatusValidator } from '../StatusValidator'; import { Status } from '../Status'; @@ -16,7 +16,7 @@ export class CustomStatusModal extends Modal { saved: boolean = false; error: boolean = false; private isCoreStatus: boolean; - constructor(public plugin: TasksPlugin, statusType: StatusConfiguration, isCoreStatus: boolean) { + constructor(public plugin: Plugin, statusType: StatusConfiguration, isCoreStatus: boolean) { super(plugin.app); this.statusSymbol = statusType.symbol; this.statusName = statusType.name; From 14d59e0302e3de0443ea196d21ef2e89e567136f Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Fri, 27 Jan 2023 06:51:34 +0000 Subject: [PATCH 3/3] refactor: Remove dependency StatusRegistry.ts > Task.ts This in turn removes the cyclic dependencies: compareByDateConfig/Settings.ts > Config/StatusSettings.ts > StatusRegistry.ts > Task.ts compareByDateConfig/Settings.ts > Config/StatusSettings.ts > StatusRegistry.ts > Task.ts > DateFallback.ts compareByDateTask.ts > DateFallback.ts compareByDateConfig/Settings.ts > Config/StatusSettings.ts > StatusRegistry.ts > Task.ts > TaskLineRenderer.ts compareByDateConfig/Settings.ts > Config/StatusSettings.ts > StatusRegistry.ts > Task.ts > TaskLineRenderer.ts > File.ts See #1591 --- src/Config/SettingsTab.ts | 5 ++++- src/StatusRegistry.ts | 13 ++++--------- tests/StatusRegistry.test.ts | 19 +++++++++---------- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/Config/SettingsTab.ts b/src/Config/SettingsTab.ts index 7f0c841c5c..ed03a39847 100644 --- a/src/Config/SettingsTab.ts +++ b/src/Config/SettingsTab.ts @@ -454,7 +454,10 @@ export class SettingsTab extends PluginSettingTab { .setCta() .onClick(async () => { const tasks = this.plugin.getTasks(); - const unknownStatuses = StatusRegistry.getInstance().findUnknownStatuses(tasks!); + const allStatuses = tasks!.map((task) => { + return task.status; + }); + const unknownStatuses = StatusRegistry.getInstance().findUnknownStatuses(allStatuses); if (unknownStatuses.length === 0) { return; } diff --git a/src/StatusRegistry.ts b/src/StatusRegistry.ts index 2070d3e9a4..3492cf12ba 100644 --- a/src/StatusRegistry.ts +++ b/src/StatusRegistry.ts @@ -1,6 +1,5 @@ import { Status } from './Status'; import { StatusConfiguration, StatusType } from './StatusConfiguration'; -import type { Task } from './Task'; /** * Tracks all the registered statuses a task can have. @@ -187,19 +186,15 @@ export class StatusRegistry { } /** - * Find any statuses in the given tasks that are not known to this registry. + * Find any statuses in the given list that are not known to this registry. * This can be used to add all unknown status types to the settings, * to save users from having to do that manually. * * Statuses are returned in the order that they are first found in the - * supplied tasks. - * @param tasks + * supplied list. + * @param allStatuses */ - public findUnknownStatuses(tasks: Task[]): Status[] { - const allStatuses = tasks.map((task) => { - return task.status; - }); - + public findUnknownStatuses(allStatuses: Status[]): Status[] { const unknownStatuses = allStatuses.filter((s) => { return !this.hasSymbol(s.symbol); }); diff --git a/tests/StatusRegistry.test.ts b/tests/StatusRegistry.test.ts index 2168e8b210..9361efca71 100644 --- a/tests/StatusRegistry.test.ts +++ b/tests/StatusRegistry.test.ts @@ -7,7 +7,6 @@ import { Status } from '../src/Status'; import { StatusConfiguration, StatusType } from '../src/StatusConfiguration'; import { Task } from '../src/Task'; import * as TestHelpers from './TestHelpers'; -import { TaskBuilder } from './TestingTools/TaskBuilder'; jest.mock('obsidian'); window.moment = moment; @@ -278,20 +277,20 @@ describe('StatusRegistry', () => { expect(registry.bySymbol('!').type).toEqual(StatusType.EMPTY); expect(registry.bySymbol('X').type).toEqual(StatusType.EMPTY); expect(registry.bySymbol('d').type).toEqual(StatusType.EMPTY); - const tasks = [ - new TaskBuilder().statusValues('!', 'Unknown', 'X', false, StatusType.TODO).build(), - new TaskBuilder().statusValues('X', 'Unknown', '!', false, StatusType.DONE).build(), - new TaskBuilder().statusValues('d', 'Unknown', '!', false, StatusType.IN_PROGRESS).build(), + const allStatuses = [ + new Status(new StatusConfiguration('!', 'Unknown', 'X', false, StatusType.TODO)), + new Status(new StatusConfiguration('X', 'Unknown', '!', false, StatusType.DONE)), + new Status(new StatusConfiguration('d', 'Unknown', '!', false, StatusType.IN_PROGRESS)), // Include some tasks with duplicate statuses, to make sure duplicates are discarded - new TaskBuilder().statusValues('!', 'Unknown', 'X', false, StatusType.TODO).build(), - new TaskBuilder().statusValues('X', 'Unknown', '!', false, StatusType.DONE).build(), - new TaskBuilder().statusValues('d', 'Unknown', '!', false, StatusType.IN_PROGRESS).build(), + new Status(new StatusConfiguration('!', 'Unknown', 'X', false, StatusType.TODO)), + new Status(new StatusConfiguration('X', 'Unknown', '!', false, StatusType.DONE)), + new Status(new StatusConfiguration('d', 'Unknown', '!', false, StatusType.IN_PROGRESS)), // Check that it does not add copies of any core statuses - new TaskBuilder().statusValues('-', 'Unknown', '!', false, StatusType.IN_PROGRESS).build(), + new Status(new StatusConfiguration('-', 'Unknown', '!', false, StatusType.IN_PROGRESS)), ]; // Act - const unknownStatuses = registry.findUnknownStatuses(tasks); + const unknownStatuses = registry.findUnknownStatuses(allStatuses); // Assert expect(unknownStatuses.length).toEqual(3);