Skip to content
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

Add support for Google searches using Google.Apis.Customsearch.v1 #537

Merged
merged 4 commits into from
Apr 29, 2023

Conversation

ScottKane
Copy link
Contributor

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

@ScottKane
Copy link
Contributor Author

@microsoft-github-policy-service agree

@lemillermicrosoft lemillermicrosoft added the kernel Issues or pull requests impacting the core kernel label Apr 20, 2023
@MovGP0
Copy link

MovGP0 commented Apr 21, 2023

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.v1 package should not be hardcoded, such that bugfixes of the library will not be missed in future builds. Only fix the MAJOR.MINOR part of the version.
  • consider setting <ImplicitUsings>true</ImplicitUsings> in order to clean up the using section
  • the apiKey and searchEngineId properties should probably be placed in an object provided by the IOptions interface

@ScottKane
Copy link
Contributor Author

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?

@adrianwyatt adrianwyatt self-assigned this Apr 25, 2023
@adrianwyatt adrianwyatt added PR: feedback to address Waiting for PR owner to address comments/questions and removed kernel Issues or pull requests impacting the core kernel labels Apr 25, 2023
@adrianwyatt
Copy link
Contributor

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?

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.

@dluc dluc assigned dluc and unassigned adrianwyatt Apr 25, 2023
@lemillermicrosoft lemillermicrosoft added the kernel Issues or pull requests impacting the core kernel label Apr 26, 2023
@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code and removed kernel Issues or pull requests impacting the core kernel labels Apr 27, 2023
@ScottKane
Copy link
Contributor Author

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.

@dluc dluc changed the base branch from main to pr537-GoogleConnector April 29, 2023 06:19
@dluc dluc merged commit 4b23dde into microsoft:pr537-GoogleConnector Apr 29, 2023
2 checks passed
dluc added a commit that referenced this pull request Apr 29, 2023
…737)

[see #537 - PR rebuilt to fix nuget dependency and merge conflicts]

### Motivation and Context
Allow the usage of Google search equivalent to current Bing search
implementation.


### Description
Uses the Google.Apis.CustomSearchAPI.v1 packages to interface with the
Google search API, this requires an API key from GCP and a custom search
engine ID

---------

Co-authored-by: Scott Kane <scottpkane94@gmail.com>
@dluc
Copy link
Collaborator

dluc commented Apr 29, 2023

@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 #737, addressed the feedback, and added you as one of the contributors in the commit log.

@ScottKane
Copy link
Contributor Author

@dluc no problem, sorry I will look out for that next time I raise a PR

dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
…icrosoft#737)

[see microsoft#537 - PR rebuilt to fix nuget dependency and merge conflicts]

### Motivation and Context
Allow the usage of Google search equivalent to current Bing search
implementation.


### Description
Uses the Google.Apis.CustomSearchAPI.v1 packages to interface with the
Google search API, this requires an API key from GCP and a custom search
engine ID

---------

Co-authored-by: Scott Kane <scottpkane94@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code PR: feedback to address Waiting for PR owner to address comments/questions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants