-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: serpapi toolkit #3489
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: serpapi toolkit #3489
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @JINO-ROHIT, It seems the unit tests are missing. |
right i was hoping if something else needs to be added before i add test cases + example |
|
hi!@JINO-ROHIT thanks for this pr!i think the advantage of serpapi lies in their integration with various search engines, allowing them to be invoked using a unified key.so maybe we can add a parameter,let user can use any search engines they want via this parameter,we can start by supporting some commonly used modules,like:GoogleSearch、Yahoo、Yandex、GoogleScholarSearch and YouTube |
|
hi @fengju0213 yeah i think the engine param is already in the PR unless im misunderstanding something, if this looks good then ill proceed to examples and test cases |
yeah I noticed you've already added this parameter, but in the proxy logic, we currently only support Google? |
|
@fengju0213 sorry for not understanding this well, can you explain what you are referring to as proxy |
I mean, although the function parameters support custom configurations, in the implementation logic we currently only support Google, right? Because I see that we only import Google — which means even if users want to configure another engine, they wouldn’t be able to use it? |
|
hi @fengju0213 ah i see what you mean, i think the import is simply poorly named by serpapi, if you change the engine to 'duckduckgo' like this, the results will be from duckduckgo |
ah i see, i read the source code of the serpapi,these search engines have been repackaged. for clarity in naming, perhaps we can use |
All the search engines inherit from this class. |
|
done! is it okay to suprres the stub warnings by doing this? |
no warries,i will help solve this |
|
@JINO-ROHIT everything looks good,lets add some examples ,then we can merge it |
|
@fengju0213 done! is this a new pre-commit that keeps failing? |
This is not related to your PR — I’ll fix it. |
|
thanks! |
LuoPengcheng12138
left a comment
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
| except ImportError: | ||
| return { | ||
| "error": "serpapi library not installed. Install with: pip install google-search-results" | ||
| } |
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.
Thanks @JINO-ROHIT for the PR, regarding the dependencies, perhaps its better to use the decorator as above. I have added the enhancement here:
#3558
Co-authored-by: a7m-1st <[email protected]> Co-authored-by: JINO ROHIT <[email protected]>



adds serpapi search toolkit , theres a lot more modules, would you like me to add anymore before i add an example and testcase for this?
reference - https://serpapi.com/search-api
fixes #3476