-
-
Notifications
You must be signed in to change notification settings - Fork 833
Add fuzzy matching support in room searches #6182
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/* | ||
Copyright 2021 The Matrix.org Foundation C.I.C. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
import { fuzzyMatch } from "../../src/utils/strings"; | ||
|
||
describe("strings", () => { | ||
describe("fuzzyMatch()", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a test that deals with numbers and symbols interspersed with characters would be nice as well, to ensure the regex cleaning works (of which there doesn't appear to be any?) and that it handles non-ascii well (including emoji). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I have added more test cases to deal with more exotic character set. I'm not sure if I know what regex cleaning is and what you expect it to do in this scenario? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "regex cleaning" is my horrible way of referring to the action the regex performs. There should be tests to make sure that the regex does the right thing, where the "right thing" is cleaning/sanitizing the input from what I can see. |
||
it("should return exact matches", () => { | ||
expect(fuzzyMatch("alice", "alice")).toBe(true); | ||
expect(fuzzyMatch("", "")).toBe(true); | ||
}); | ||
it("supports non ASCII characters", () => { | ||
expect(fuzzyMatch("éè?]!", "é]")).toBe(true); | ||
expect(fuzzyMatch("안녕하세요", "녕세")).toBe(true); | ||
}); | ||
it("works with emojis", () => { | ||
expect(fuzzyMatch("BoB 🥳", "🥳")).toBe(true); | ||
expect(fuzzyMatch("1️⃣2️⃣3️⃣4️⃣5️⃣", "1️⃣3️⃣5️⃣")).toBe(true); | ||
}); | ||
it("matches across multiple words", () => { | ||
expect(fuzzyMatch("lorem ipsum dolor sit amet", "lidsa")).toBe(true); | ||
}); | ||
it("doesn't match over multiple lines", () => { | ||
expect(fuzzyMatch(`Hello | ||
World`, "HW")).toBe(false); | ||
}) | ||
it("should be case sensitive", () => { | ||
expect(fuzzyMatch("BoB", "bOb")).toBe(false); | ||
}); | ||
it("should match anywhere in the string", () => { | ||
expect(fuzzyMatch("Alice", "Al")).toBe(true); | ||
expect(fuzzyMatch("Alice", "lic")).toBe(true); | ||
expect(fuzzyMatch("Alice", "ce")).toBe(true); | ||
}); | ||
it("should allow for gaps in search", () => { | ||
expect(fuzzyMatch("Alice", "Aie")).toBe(true); | ||
expect(fuzzyMatch("Alice", "Ale")).toBe(true); | ||
expect(fuzzyMatch("Alice", "lc")).toBe(true); | ||
}) | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how we feel about this more generally, but making this
private fuzzy = false
is slightly cleaner and works well with IDEs. The options approach does allow for expandability, however this is internal API and can be changed with relative ease.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand the change that you're suggesting.
As far as I can tell I have created
private fuzzy = false
and this options parameter is only here to let a developer choose the type of matching theNameFilterCondition
should applyCould you show me an example of what you had in mind instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, merging the field declaration with the constructor declaration:
constructor(private fuzzy = false) {
It's a little bit cleaner for the API contract this part of the code is after.