Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1387 (2022-12-10 22:27:53)

fix: make parsing of priority filter more precise and more forgiving
<!--- Provide a general summary of your changes in the Title above -->

# Description

Improve the parsing of the `priority` instruction

1. **parsing is more precise**: lines such as the following were accepted previously, and now are not:
    - `priority is abovemedium`
    - `priority is above medium-with-nonsense-at-end` (user may have thought that could add something meaningful after `medium` here)
1. **parsing is more forgiving**: words in `priority` filters may be separated by multiple spaces, and tab characters are recognised as whitespace too.


## Motivation and Context

Responding to really helpful code review feedback from @cito in #1365, which turned out to apply to the existing code, so I decided to put it in a separate PR.

## How has this been tested?

- Running existing tests
- Adding new tests

Each individual change to the regular expression in `PriorityField` was made after a failing test was added, to be sure that each change was needed and that it made the intended change in behaviour.

(Doing this found an error in a newly-added test along the way)

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Changes visible to users:

- [x] **Bug fix** (prefix: `fix` - non-breaking change which fixes an issue)

Internal changes:

- [ ] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] 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](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#updating-documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#maintaining-the-tests).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/Query/Filter/PriorityField.ts b/src/Query/Filter/PriorityField.ts
index f3bcfc27..19c5511f 100644
--- a/src/Query/Filter/PriorityField.ts
+++ b/src/Query/Filter/PriorityField.ts
@@ -4,13 +4,17 @@ import { Field } from './Field';
 import { Filter, FilterOrErrorMessage } from './Filter';
 
 export class PriorityField extends Field {
-    private static readonly priorityRegexp = /^priority (is )?(above|below|not)? ?(low|none|medium|high)/;
+    // The trick in the following to manage whitespace with optional values
+    // is to capture them in Nested Capture Groups, like this:
+    //  (leading-white-space-in-outer-capture-group(values-to-use-are-in-inner-capture-group))
+    // The capture groups are numbered in the order of their opening brackets, from left to right.
+    private static readonly priorityRegexp = /^priority(\s+is)?(\s+(above|below|not))?(\s+(low|none|medium|high))$/;
 
     createFilterOrErrorMessage(line: string): FilterOrErrorMessage {
         const result = new FilterOrErrorMessage(line);
         const priorityMatch = Field.getMatch(this.filterRegExp(), line);
         if (priorityMatch !== null) {
-            const filterPriorityString = priorityMatch[3];
+            const filterPriorityString = priorityMatch[5];
             let filterPriority: Priority | null = null;
 
             switch (filterPriorityString) {
@@ -35,7 +39,7 @@ export class PriorityField extends Field {
 
             let explanation = line;
             let filter;
-            switch (priorityMatch[2]) {
+            switch (priorityMatch[3]) {
                 case 'above':
                     filter = (task: Task) => task.priority.localeCompare(filterPriority!) < 0;
                     break;


Test patch:
diff --git a/tests/Query/Filter/PriorityField.test.ts b/tests/Query/Filter/PriorityField.test.ts
index 8e8400e7..27138397 100644
--- a/tests/Query/Filter/PriorityField.test.ts
+++ b/tests/Query/Filter/PriorityField.test.ts
@@ -3,9 +3,10 @@ import { TaskBuilder } from '../../TestingTools/TaskBuilder';
 import { testFilter } from '../../TestingTools/FilterTestHelpers';
 import { PriorityField } from '../../../src/Query/Filter/PriorityField';
 
-import { toHaveExplanation } from '../../CustomMatchers/CustomMatchersForFilters';
+import { toBeValid, toHaveExplanation } from '../../CustomMatchers/CustomMatchersForFilters';
 
 expect.extend({
+    toBeValid,
     toHaveExplanation,
 });
 
@@ -83,10 +84,26 @@ describe('priority is not', () => {
     });
 });
 
+describe('priority parses various whitespace combinations', () => {
+    // Not tested here: Query strips off trailing whitespace, so spaces at start
+    // and end of the instruction do not need testing
+    it.each(['priority  is low', 'priority is  low', 'priority is  above low', 'priority\tis\tabove\tlow'])(
+        'white space variation: "%s"',
+        (filter: string) => {
+            const filterOrError = new PriorityField().createFilterOrErrorMessage(filter);
+            expect(filterOrError).toBeValid();
+        },
+    );
+});
+
 describe('priority error cases', () => {
-    it('priority is something', () => {
+    it.each([
+        'priority is no-such-priority',
+        'priority is abovemedium',
+        'priority is above medium-with-nonsense-at-end',
+    ])('filter: "%s"', (input: string) => {
         const field = new PriorityField();
-        const filter = field.createFilterOrErrorMessage('priority is no-such-priority');
+        const filter = field.createFilterOrErrorMessage(input);
         expect(filter.filterFunction).toBeUndefined();
         expect(filter.error).toBe('do not understand query filter (priority)');
     });