Search: Fix word exclusion in client side search #13893
Conversation
|
Regarding the CI: I don't see any changes on my PR which would trigger the current CI fail. Is this a common issue? TBH it does not look like it's affecting the master branch too 🤔. I'm a little aimless what to do here. |
That's OK, yep - I believe that is due to bug #13886 (in progress, potentially to be fixed by #13883). |
…earch-terms # Conflicts: # CHANGES.rst
|
A delayed thought here: adding the exclusion operator to hyphenated query terms could cause unexpected results. For example, the query |
| [...excludedTerms].some((excludedTerm) => { | ||
| // Both mappings will contain either a single integer or a list of integers. | ||
| // Converting them to lists makes the comparison more readable. | ||
| let excludedTermFiles = [].concat(terms[excludedTerm]); | ||
| let excludedTitleFiles = [].concat(titleTerms[excludedTerm]); | ||
| return ( | ||
| excludedTermFiles.includes(file) | ||
| || excludedTitleFiles.includes(file) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
| [...excludedTerms].some((excludedTerm) => { | |
| // Both mappings will contain either a single integer or a list of integers. | |
| // Converting them to lists makes the comparison more readable. | |
| let excludedTermFiles = [].concat(terms[excludedTerm]); | |
| let excludedTitleFiles = [].concat(titleTerms[excludedTerm]); | |
| return ( | |
| excludedTermFiles.includes(file) | |
| || excludedTitleFiles.includes(file) | |
| ); | |
| }) | |
| [...excludedTerms].some( | |
| (term) => | |
| terms[term] === file | |
| || titleTerms[term] === file | |
| || (terms[term] || []).includes(file) | |
| || (titleTerms[term] || []).includes(file), | |
| ) |
Do we need this set of excludedTerms filtering changes? I would suggest not modifying these lines unless strictly necessary. Tests continue to pass when I revert this.
I acknowledge that the break to continue fixup is important though.
There was a problem hiding this comment.
Ok, I'll need to add another test then. The last conditions raise an error in some cases. I'll add it soon.
There was a problem hiding this comment.
Hi @jayaddison,
I added the "should exclude results where the file index is above 0." test case which raises an TypeError with the old code. It's very specific for the old implementation.
The error occurs every time the terms[term] statement is a positive integer and not equal to the file variable. I think this bug was introduced when the datastructures were changed from actual file paths/names to file ids.
The strings usually evaluate to false in this condition, but integers above 0 evaluate to true.
No change in functionality intended. Only update the automatically generated files which might be forgotten.
Use lookbehind to split words only if a hyphon is contained inside a word. Co-authored-by: James Addison <[email protected]>
Hi @jayaddison,
Altogether, I think that this is a very valid concern. Still, I would not address this in this MR but rather open another one to get the bugfix out quickly 😁 |
3 participants
Purpose
The built-in search is capable of excluding search terms. Thats a great feature which would make the search a lot better!
Unfortunately, there are two blocking components:
splitQuerywill discard hyphens which define the excluded termsperformTermsSearchwill abort the search if any excluded term is matchedReferences
Closes #13892