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.

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

refactor: Implement new preprocessor for Boolean searches #2758

Merged
merged 10 commits into from
Apr 9, 2024

Conversation

claremacrae
Copy link
Collaborator

Description

Note: this is labelled as a refactor as the new code is not yet used in the Tasks plugin.

This extends BooleanPreprocessor to add a more sophisticated implementation for preprocessing Boolean/combined filters.

  • Add `BooleanPreprocessorResult
  • Add BooleanPreprocessor.preprocessExpression()

This change - and especially the new method isAFilter() - is the result of several weeks and many, many hours of trying to maintain existing working searches, whilst also adding support for filters that contain the following characters:

  • (
  • )
  • "

Caveats

The one current caveat with this implementation is that it does not work for filters the end with any of the characters ()", as they are swallowed up and treated as delimiters.

So for example, this filter:

(description includes "maybe")

is interpreted as this simplified line - note the stray ":

(f1")

where f1 is this - note the missing ":

description includes "maybe

Motivation and Context

Likely the last refactor before addressing:

How has this been tested?

  • Adding and updating tests
  • Diffing the output of these two files:
    • BooleanField.test.boolean_query_-_exhaustive_tests_preprocess.approved.txt
    • BooleanField.test.boolean_query_-_exhaustive_tests_preprocess_-_rewrite.approved.txt

Screenshots (if appropriate)

Types of changes

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)

Checklist

  • My code follows the code style of this project and passes yarn run lint.
  • My change has adequate Unit Test coverage.

Terms

…are all capitals

This is tested, but I decided to use Arlo Belshee's @ symbol to signify the scale of the change.

This set of regular expressions was built up empirically through a lot of iteration,
over a very thorough set of sample Boolean filters, in order to detect all the outputs
from splitLine() that were not actually Tasks filters.

In this commit, I tried breaking it down in to smaller chunks, but I ended up feeling
that it was more useful to commit the corrected implementation, so that the changes to
tests in this commit usefully showed the small change in behaviour of the rewrite.
…rResult

It was useful whilst implementing the new preprocessing algorithm
but is now just clutter in snapshots and approved files.
@claremacrae claremacrae added type: internal Only regards development or contributing scope: query logic Boolean combinations of filters - and, or, not labels Apr 9, 2024
@claremacrae claremacrae merged commit 658e19d into main Apr 9, 2024
2 checks passed
@claremacrae claremacrae deleted the new-boolean-preprocessor branch April 9, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: query logic Boolean combinations of filters - and, or, not type: internal Only regards development or contributing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant