-
Notifications
You must be signed in to change notification settings - Fork 23
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
FEAT: Enable Algolia integration #145
Conversation
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 looking really great! Thanks for writing some detailed tests. There is definitly some refactoring as we add more search engines that could be done in seperate PR's. I have a couple of comments...
@sumitsarkar are you planning on doing the work in Quepid as well? I'd love to have you pick up that side as well, and then when the PR for Quepid is done, we merge both of these at the same time, I bump this splainer-search, bump it in Quepid, and then cut a release? I guess I don't know how comfortable you are modifying Quepid... Basically search for the word "vectara" to find all the places that need changing! |
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.
LGTM, pending figuring out if we ship it now, or first get the Quepid integration working (in case we need to modify this some more), and then ship it...
I'd be in favor of preparing a Quepid integration working first. I have a working version already. Will clean that up and prepare a PR. |
|
||
function constructObjectQueryUrl(url) { | ||
var urlObject = new URL(url); | ||
urlObject.pathname = '/1/indexes/*/objects'; |
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.
does the /1/ imply there is a /2/?? or will it alwyas be like this?
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.
It is the API version number. It could change in the future. But it's unlikely the version will be deprecated and removed quickly. There should be time for migration to new API version number.
Source
working through another PR, and then hope to get this in first thing this week... once I have tested with my demo algolia account! |
explain
,highlight
,snippet
and ability to retrieve document viaid
.Closes #144