-
Notifications
You must be signed in to change notification settings - Fork 79
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
Swap from exact substring matches to each individual word matching #145
Conversation
@@ -271,23 +271,24 @@ angular | |||
'tags': 'array', | |||
'arguments': 'array', | |||
}; | |||
var search = new RegExp(val, "i") |
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.
This was never used
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.
This is really cool @joellabes !!
I'll wait until @drewbanin can give the JS a once-over, likely next week
ha - oops - thanks for your patience @joellabes - giving this a look now :D |
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 love what you're going for here, but I think it's about time that we giving searching some more love! What do you think about leveraging something like fusejs here and deleting a lot of this searching logic? I kicked around fuse.js, and it looks to me like it will match the query from the original issue (#143) out of the box!
Joel, if this is something you're interested in pursuing, I'd be really happy to offer some guidance! If not, I imagine it's something that we could get to in the reasonably near future. Let me know either way!
I'd be so excited for search to get some love - I had an urge to rework the weighting process last week as well. Happy to have a blast with Fuse. I had dramas getting dbt running locally (Windows 🙄) but will give it another whirl - if it doesn't magically work I might have to get a hand on the initial setup! (Might be a couple of weeks though, so don't feel obliged to wait for me if the mood strikes you) |
I've started looking into this on #162 - still a long way from it being ready, but I'm going to close this one for now. |
OK so 162 wound up being non-performant and not very good at fuzzy search. Not a good combo. Since this has been sitting around waiting for a year and a bit, and works just fine, I'm going to reopen it and hopefully we can get it out into a future docs release. If someone who knows more JS wants to have a swing at fuse or some other proper search lib, I'm into it. But in general, dbt docs searches are only a couple of words and it's not unreasonable to just require accurate typing 😅 |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Joel Labes.
|
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.
Just a couple comments about using lodash functions, nothing else stands out 👍
src/app/components/search/search.js
Outdated
return string.replace(/[.*+\-?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string | ||
} | ||
|
||
function splitQuery(query){ |
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.
Nitpick: a better name for this might be getQueryTokens
, since it is unclear what the query is being split on without reading this function. Also, if you use lodash's words
method, I think this function becomes unnecessary _.words(text)
.
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.
Yep that's a better name! Have kept the function for lowercasing purposes but have swapped out all the icky split stuff for _.words
Tested this out and it seems like a big quality of life improvement! 🙌 I'll take it for another spin once you implement @stu-k's suggestions. Your commit from October 13, 2020 is causing the cla bot to fail because it's associated with your |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Joel Labes.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Joel Labes.
|
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
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.
Great work! Thanks for staying on it and pushing it through. The search improvements are so so satisfying.
resolves #143
Description
This is a first attempt at considering each word entered in a search individually, as opposed to requiring a perfect match (
model my
ormy model
should now findmy_model
, where earlier only an exact search, including underscores, would have worked).my_model
still works of course.I couldn't get the Docs site running locally (from looking at the stack traces, seemed to be a bunch of issues with webpack, node versions, python versions...) but
I have tested that each individual piece of work behaves correctly outside of the wider Docs contextI tried it in the Netlify CI preview and it works correctly.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.