-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
community[minor]: Add DuckDuckGoSearch tool #4943
community[minor]: Add DuckDuckGoSearch tool #4943
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -445,6 +447,9 @@ | |||
"dria": { |
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.
Hey there! I noticed that the recent PR added an optional dependency for "duck-duck-scrape" in the package.json file. This comment is to flag the change for maintainers to review, as it may impact the project's dependencies. Keep up the great work!
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.
By the way, I can put the light version of the implementation of the search results inside the source code of the tools itself, if you prefer to not have or stick to the fewer third-party dependencies. I was considering that as an option, but I've opted to integrate DuckDuckGo via a well-established third-party library to leverage its maturity and likelihood of timely updates in response to any changes by DuckDuckGo
I'm open to discussing which approach aligns best with our project's priorities and am ready to adjust based on our collective feedback.
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.
Overall looks good, I left a couple comments. This also needs a page in the docs.
I pushed up some changes with the nits I mentioned & docs to a branch here which you can merge into yours!
Thanks for adding this!
/** | ||
* Class for interacting with the DuckDuckGoSearch engine | ||
* It extends the base Tool class and implements the _call method to | ||
* perform the retrieve operation. | ||
* | ||
*/ |
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 should be directly above export class DuckDuckGoSearch
so it shows up as the docstring for the class. Also, nit on the content
/** | |
* Class for interacting with the DuckDuckGoSearch engine | |
* It extends the base Tool class and implements the _call method to | |
* perform the retrieve operation. | |
* | |
*/ | |
/** | |
* Class for interacting with the DuckDuckGo search engine | |
* It extends the base Tool class to perform retrieval. | |
*/ |
* */ | ||
|
||
searchOptions?: SearchOptions; | ||
/** | ||
* @default 10 | ||
* The maximum number of results to return from the search. | ||
* Limiting to 10 to avoid context overload. | ||
* */ |
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.
Any JS/TSDoc should be directly above the code it's for
* */ | |
searchOptions?: SearchOptions; | |
/** | |
* @default 10 | |
* The maximum number of results to return from the search. | |
* Limiting to 10 to avoid context overload. | |
* */ | |
*/ | |
searchOptions?: SearchOptions; | |
/** | |
* @default 10 | |
* The maximum number of results to return from the search. | |
* Limiting to 10 to avoid context overload. | |
*/ |
@bracesproul Many thanks for the review and your suggestions in the branch. |
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, thanks for adding!!
@@ -18,6 +18,7 @@ export const config = { | |||
"convex/values", | |||
"@rockset/client/dist/codegen/api.js", | |||
"discord.js", | |||
"duck-duck-scrape", |
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 don't think this is strictly necessary?
Thank you! |
Context:
Here is another try to add the DuckDuckGo as a community search tool.
Duplicate of - one that unfortunately still stays in Open state that was reviewed by @dqbd
I am open to any comments and suggestions for this merge request. I value your time and am willing to make the necessary changes to expedite its acceptance into the main branch as I consider these tools a valuable addition to the TS version of the community toolset.
Thank you for the opportunity to contribute to the project!
Change description:
DuckDuckGo tool that uses the existing mature under the hood.
Tools take the same options as the library it uses, plus the possibility to limit the max number of return results, as I experience that the initial amount might overload the context buffer with less relevant results from the end of the list
The DuckDuckGo tool inherits all options from the underlying library, adding a feature to limit the maximum number of results returned. This addresses the possible issue of context overload by filtering out less relevant search results typically found at the end of the list.