Skip to content

Add scoring and sorting to sequence matcher#7367

Merged
donkawechico merged 12 commits intohome-assistant:devfrom
donkawechico:fuzzy_sequence_scoring
Oct 20, 2020
Merged

Add scoring and sorting to sequence matcher#7367
donkawechico merged 12 commits intohome-assistant:devfrom
donkawechico:fuzzy_sequence_scoring

Conversation

@donkawechico
Copy link
Copy Markdown
Contributor

@donkawechico donkawechico commented Oct 16, 2020

Proposed change

Add scoring to the filter algorithm to make more relevant filters appear first in the list.

The current simple sequence matching algorithm doesn't score matches, so if a low-quality result is higher alphabetically, it will appear first, resulting in a confusing experience.

For example, "media" will match "automation.choose_duplicate" before it matches "media_player.bedroom" simply because "automation" starts with an "a".

This PR ports over VS Code's implementation of filter for its quick access bar.

The filter code is lazy-loaded after user launches quick bar.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@donkawechico donkawechico changed the title Replace sequence matcher with VS Code's score-based implementation Add scoring and sorting to sequence matcher Oct 16, 2020
@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch 3 times, most recently from 86c6dc4 to 3e82916 Compare October 16, 2020 23:09
zsarnett
zsarnett previously approved these changes Oct 16, 2020
Copy link
Copy Markdown
Contributor

@zsarnett zsarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we stole this from VSCode. Lets make a comment at the top of each "stolen" file or function that is copied.

@zsarnett
Copy link
Copy Markdown
Contributor

I meant to comment not approve...

@zsarnett zsarnett self-requested a review October 16, 2020 23:35
Copy link
Copy Markdown
Contributor

@zsarnett zsarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is going to be difficult to review. Just as a quick glance. Do we need ALL of the checks?are we comparing case? like if case the same then if it isn't the same and comparing both. We should probably only do one or the other. But again only glanced. Vscodes implementation may be more powerful than we need.

@donkawechico
Copy link
Copy Markdown
Contributor Author

donkawechico commented Oct 17, 2020

Do we need ALL of the checks?are we comparing case?

We definitely don't need all of the checks. But we don't need all the features of fuse.js, but we import them all anyway. This is sort of just a library import.

I realize that's not a water-tight argument. I'm just not sure I see the value in spending a long time deciphering the code, surgically removing the features we aren't currently using, only to later discover they'd be quite helpful.

All that being said, now that I know we can get what we want using only fuzzyScore and matchesSubString, I could at least go through and remove anything unrelated to those two functions.

@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch 2 times, most recently from ca6f8fc to ce9da22 Compare October 17, 2020 00:38
@donkawechico
Copy link
Copy Markdown
Contributor Author

Since we stole this from VSCode. Lets make a comment at the top of each "stolen" file or function that is copied.

Confession time. HA is my first meaningful contribution to open source anything. I don't know the proper way to apply credit, so I just replaced the existing microsoft copyright declaration at the top of each borrowed file with the contents of VS Code's license.txt.

I didn't put any MS credit in sequence-matching.ts because that's all my code.

Is that correct?

@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch 5 times, most recently from 9e0e757 to 786b096 Compare October 18, 2020 01:37
@donkawechico donkawechico marked this pull request as ready for review October 18, 2020 02:36
@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch from d21ab89 to 03c6a29 Compare October 19, 2020 23:06
@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch 2 times, most recently from 92e2cb3 to 3efa03d Compare October 19, 2020 23:15
@donkawechico donkawechico force-pushed the fuzzy_sequence_scoring branch from 3efa03d to 2581c01 Compare October 20, 2020 02:56
@donkawechico donkawechico merged commit 6f2a759 into home-assistant:dev Oct 20, 2020
@bramkragten bramkragten mentioned this pull request Oct 21, 2020
@vanackej vanackej mentioned this pull request Apr 25, 2022
9 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants