semantic-kernel
semantic-kernel copied to clipboard
Add support for Google searches using Google.Apis.Customsearch.v1
Motivation and Context
Allow the usage of Google search equivalent to current Bing search implementation.
Description
Uses the Google.Apis.Customsearch.v1 packages to interface with the Google search API, this requires an API key from GCP and a custom search engine ID
Contribution Checklist
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with
dotnet format - [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone :smile:
@microsoft-github-policy-service agree
A few thoughts:
- the
this.keywords can be safely removed everywhere, since they do not contribute anything. - the exact version of the
Google.Apis.Customsearch.v1package should not be hardcoded, such that bugfixes of the library will not be missed in future builds. Only fix theMAJOR.MINORpart of the version. - consider setting
<ImplicitUsings>true</ImplicitUsings>in order to clean up the using section - the
apiKeyandsearchEngineIdproperties should probably be placed in an object provided by theIOptionsinterface
The original code written was without this. however StyleCop was enforcing the use of them so I assumed this was required. Happy to make any changes required as soon as we can iron out the listed questions surrounding the interface and naming conventions. The BingConnector also takes a string apiKey so should that change to an Option type too?
The original code written was without
this.however StyleCop was enforcing the use of them so I assumed this was required. Happy to make any changes required as soon as we can iron out the listed questions surrounding the interface and naming conventions. TheBingConnectoralso takes a stringapiKeyso should that change to an Option type too?
Yes, current repo style guides are to use "this.". You should also run "dotnet format" in the "/dotnet" directory to gets most of the style adjustments made for free. This will help the "dotnet-format" gate pass.
Ok I ran dotnet format as requested, I think ommitting Search from the Bing/Google connectors is a better descision, they both implement IWebSearchEngineConnector which can tell you're working with a search engine while leaving the posibility for the same connectors to implement different interfaces in the future, e.g both could implement an IWebMapsConnector or something, at which point having Search in the name might not be so sensible.
@ScottKane thanks for bringing this in! Unfortunately you unchecked the "allow maintainers to edit the PR" and we could not address some important changes, e.g. the dependency on an old nuget. I tried working around the block but between this and several conflicts it was taking too long (and apparently impossible), so I recreated your PR here https://github.com/microsoft/semantic-kernel/pull/737, addressed the feedback, and added you as one of the contributors in the commit log.
@dluc no problem, sorry I will look out for that next time I raise a PR