From 21428e76638b4ef83f7dae63cb8d07bef6006980 Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Tue, 19 Mar 2024 11:25:37 +0000 Subject: [PATCH 1/5] fix: 'explain' always include the original instruction in Boolean filters If the instruction differs from the explanation. Fixes #2707 --- src/Query/Filter/BooleanField.ts | 13 ++++++++++++- ...lean_combinations.approved.explanation.text | 3 ++- ...ery_-_exhaustive_tests_explain.approved.txt | 18 ++++++++++++------ tests/Query/Filter/BooleanField.test.ts | 7 +++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/Query/Filter/BooleanField.ts b/src/Query/Filter/BooleanField.ts index f5e47548ea..561afd3738 100644 --- a/src/Query/Filter/BooleanField.ts +++ b/src/Query/Filter/BooleanField.ts @@ -200,7 +200,18 @@ export class BooleanField extends Field { throw Error('null token value'); // This should not happen } const filter = this.subFields[token.value.trim()]; - explanationStack.push(filter.explanation); + const explanation = this.simulateExplainFilter(filter); + explanationStack.push(explanation); + } + + private simulateExplainFilter(filter: Filter) { + // In our caller, the explanationStack keeps the explanations but not the instruction lines. + // So to replicate the logic in Filter.explainFilterIndented(), we may need to add the + // instruction to the explanation. + const needToShowInstruction = filter.instruction !== filter.explanation.asString(); + return needToShowInstruction + ? new Explanation(filter.instruction + ' =>', [filter.explanation]) + : filter.explanation; } private explainOperator(token: Token, explanationStack: Explanation[]) { diff --git a/tests/Query/Explain/DocsSamplesForExplain.test.explain_boolean_combinations.approved.explanation.text b/tests/Query/Explain/DocsSamplesForExplain.test.explain_boolean_combinations.approved.explanation.text index 9afa33ea25..ecd2387c97 100644 --- a/tests/Query/Explain/DocsSamplesForExplain.test.explain_boolean_combinations.approved.explanation.text +++ b/tests/Query/Explain/DocsSamplesForExplain.test.explain_boolean_combinations.approved.explanation.text @@ -4,7 +4,8 @@ Explanation of this Tasks code block query: (due before tomorrow) AND (is recurring) => AND (All of): - due date is before 2022-10-22 (Saturday 22nd October 2022) + due before tomorrow => + due date is before 2022-10-22 (Saturday 22nd October 2022) is recurring No grouping instructions supplied. diff --git a/tests/Query/Filter/BooleanField.test.boolean_query_-_exhaustive_tests_explain.approved.txt b/tests/Query/Filter/BooleanField.test.boolean_query_-_exhaustive_tests_explain.approved.txt index 76630e9685..1280609084 100644 --- a/tests/Query/Filter/BooleanField.test.boolean_query_-_exhaustive_tests_explain.approved.txt +++ b/tests/Query/Filter/BooleanField.test.boolean_query_-_exhaustive_tests_explain.approved.txt @@ -219,7 +219,8 @@ Result: (description includes d1) AND (priority medium) => AND (All of): description includes d1 - priority is medium + priority medium => + priority is medium -------------------------------------------------------- @@ -247,7 +248,8 @@ Result: AND (All of): description includes d1 NOT: - priority is medium + priority medium => + priority is medium -------------------------------------------------------- @@ -274,7 +276,8 @@ Result: OR (At least one of): description includes d1 description includes d2 - priority is medium + priority medium => + priority is medium -------------------------------------------------------- @@ -287,7 +290,8 @@ Result: (description includes d1) OR (priority medium) => OR (At least one of): description includes d1 - priority is medium + priority medium => + priority is medium -------------------------------------------------------- @@ -315,7 +319,8 @@ Result: OR (At least one of): description includes d1 NOT: - priority is medium + priority medium => + priority is medium -------------------------------------------------------- @@ -341,7 +346,8 @@ Result: (description includes d1) XOR (priority medium) => XOR (Exactly one of): description includes d1 - priority is medium + priority medium => + priority is medium -------------------------------------------------------- diff --git a/tests/Query/Filter/BooleanField.test.ts b/tests/Query/Filter/BooleanField.test.ts index 80f20db0c2..0acc93268e 100644 --- a/tests/Query/Filter/BooleanField.test.ts +++ b/tests/Query/Filter/BooleanField.test.ts @@ -220,7 +220,7 @@ describe('boolean query - explain', () => { return new Explainer(indentation).explainFilters(query1); } - it.failing('should make multi-line explanations consistent in and out of Boolean filter - issue #2707', () => { + it('should make multi-line explanations consistent in and out of Boolean filter - issue #2707', () => { // See https://github.com/obsidian-tasks-group/obsidian-tasks/issues/2707 const filter = 'description regex matches /buy/i'; @@ -231,7 +231,6 @@ describe('boolean query - explain', () => { const filter2Explanation = explainFilters(0, filter2); // Ensure that the full standalone explanation of filter1 is present in its explanation in the Boolean query: - // TODO: Make this test pass: expect(filter2Explanation).toContain(filter1Explanation); // These inline snapshots are provided for ease of visualising the behaviour: @@ -241,12 +240,12 @@ describe('boolean query - explain', () => { " `); - // TODO: Include the original regex source line in this string: expect(filter2Explanation).toMatchInlineSnapshot(` "( description regex matches /buy/i ) AND ( path includes {{query.file.path}} ) => ( description regex matches /buy/i ) AND ( path includes some/sample/note.md ) => AND (All of): - using regex: 'buy' with flag 'i' + description regex matches /buy/i => + using regex: 'buy' with flag 'i' path includes some/sample/note.md " `); From 9b1926b8450c3a1d2ffffbcb65d826434f2f9f16 Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Tue, 19 Mar 2024 11:34:59 +0000 Subject: [PATCH 2/5] docs: Update 'Explaining Queries' page for fix of #2707 --- docs/Queries/Explaining Queries.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/Queries/Explaining Queries.md b/docs/Queries/Explaining Queries.md index bc8f0f240e..96e9d10045 100644 --- a/docs/Queries/Explaining Queries.md +++ b/docs/Queries/Explaining Queries.md @@ -114,7 +114,8 @@ Explanation of this Tasks code block query: (due before tomorrow) AND (is recurring) => AND (All of): - due date is before 2022-10-22 (Saturday 22nd October 2022) + due before tomorrow => + due date is before 2022-10-22 (Saturday 22nd October 2022) is recurring No grouping instructions supplied. From 9da4a68ff684d5d06632f232a6cbd6090a0d5545 Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Tue, 19 Mar 2024 17:56:40 +0000 Subject: [PATCH 3/5] refactor: - Move body of BooleanField.simulateExplainFilter() to Filter Having realised it entirely worked on the filter parameter - and also it brigs together two similar uses of the string ' =>'. --- src/Query/Filter/BooleanField.ts | 5 +---- src/Query/Filter/Filter.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Query/Filter/BooleanField.ts b/src/Query/Filter/BooleanField.ts index 561afd3738..4a6b803ad1 100644 --- a/src/Query/Filter/BooleanField.ts +++ b/src/Query/Filter/BooleanField.ts @@ -208,10 +208,7 @@ export class BooleanField extends Field { // In our caller, the explanationStack keeps the explanations but not the instruction lines. // So to replicate the logic in Filter.explainFilterIndented(), we may need to add the // instruction to the explanation. - const needToShowInstruction = filter.instruction !== filter.explanation.asString(); - return needToShowInstruction - ? new Explanation(filter.instruction + ' =>', [filter.explanation]) - : filter.explanation; + return filter.simulateExplainFilter(filter); } private explainOperator(token: Token, explanationStack: Explanation[]) { diff --git a/src/Query/Filter/Filter.ts b/src/Query/Filter/Filter.ts index 61dfe0ad8c..8b00f3baa9 100644 --- a/src/Query/Filter/Filter.ts +++ b/src/Query/Filter/Filter.ts @@ -1,5 +1,5 @@ import type { Task } from '../../Task/Task'; -import type { Explanation } from '../Explain/Explanation'; +import { Explanation } from '../Explain/Explanation'; import type { SearchInfo } from '../SearchInfo'; import { Statement } from '../Statement'; @@ -64,4 +64,11 @@ export class Filter { return `${explainedStatement} =>\n${explanation.asString(indent + ' ')}\n`; } } + + public simulateExplainFilter(filter: Filter) { + const needToShowInstruction = filter.instruction !== filter.explanation.asString(); + return needToShowInstruction + ? new Explanation(filter.instruction + ' =>', [filter.explanation]) + : filter.explanation; + } } From d78f1f1d43015fc7a0406b5f9056a08eebbcf922 Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Tue, 19 Mar 2024 17:58:52 +0000 Subject: [PATCH 4/5] refactor: . Remove unnecessary parameter from Filter.simulateExplainFilter() --- src/Query/Filter/BooleanField.ts | 2 +- src/Query/Filter/Filter.ts | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/Query/Filter/BooleanField.ts b/src/Query/Filter/BooleanField.ts index 4a6b803ad1..cd77ec1b3f 100644 --- a/src/Query/Filter/BooleanField.ts +++ b/src/Query/Filter/BooleanField.ts @@ -208,7 +208,7 @@ export class BooleanField extends Field { // In our caller, the explanationStack keeps the explanations but not the instruction lines. // So to replicate the logic in Filter.explainFilterIndented(), we may need to add the // instruction to the explanation. - return filter.simulateExplainFilter(filter); + return filter.simulateExplainFilter(); } private explainOperator(token: Token, explanationStack: Explanation[]) { diff --git a/src/Query/Filter/Filter.ts b/src/Query/Filter/Filter.ts index 8b00f3baa9..3688ea9015 100644 --- a/src/Query/Filter/Filter.ts +++ b/src/Query/Filter/Filter.ts @@ -65,10 +65,8 @@ export class Filter { } } - public simulateExplainFilter(filter: Filter) { - const needToShowInstruction = filter.instruction !== filter.explanation.asString(); - return needToShowInstruction - ? new Explanation(filter.instruction + ' =>', [filter.explanation]) - : filter.explanation; + public simulateExplainFilter() { + const needToShowInstruction = this.instruction !== this.explanation.asString(); + return needToShowInstruction ? new Explanation(this.instruction + ' =>', [this.explanation]) : this.explanation; } } From 896ec9793ea2dced02dd7c402b338e7284c76e58 Mon Sep 17 00:00:00 2001 From: Clare Macrae Date: Tue, 19 Mar 2024 18:00:03 +0000 Subject: [PATCH 5/5] refactor: . Replace ?: with if-else - for clarity --- src/Query/Filter/Filter.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Query/Filter/Filter.ts b/src/Query/Filter/Filter.ts index 3688ea9015..fd73e651f0 100644 --- a/src/Query/Filter/Filter.ts +++ b/src/Query/Filter/Filter.ts @@ -67,6 +67,10 @@ export class Filter { public simulateExplainFilter() { const needToShowInstruction = this.instruction !== this.explanation.asString(); - return needToShowInstruction ? new Explanation(this.instruction + ' =>', [this.explanation]) : this.explanation; + if (needToShowInstruction) { + return new Explanation(this.instruction + ' =>', [this.explanation]); + } else { + return this.explanation; + } } }