Skip to content

fix issue 508 - #556

Merged
Shawn Callegari (shawncal) merged 13 commits into
microsoft:mainfrom
feiyun0112:issue-508
May 6, 2023
Merged

Shawn Callegari (shawncal) merged 13 commits into
microsoft:mainfrom
feiyun0112:issue-508

Conversation

@feiyun0112

@feiyun0112 feiyun0112 commented Apr 20, 2023 •

Copy link
Copy Markdown
Contributor

Motivation and Context

fix issue #508

Description

WebSearch Results using the Bing Connector has a hard code value of count=1.

We need to return more than 1 result or should be able to pass count of result requested

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

@lemillermicrosoft Lee Miller (lemillermicrosoft) added the kernel Issues or pull requests impacting the core kernel label Apr 20, 2023

@dluc Devis Lucato (dluc) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few things before merging:

  • provide PR title and description, pls see PR template
  • fix code style (see rider/resharper)
  • make sure this follow the same naming convention and approach used for "recall"

@adrianwyatt Adrian Bonar (adrianwyatt) added PR: feedback to address Waiting for PR owner to address comments/questions .NET Issue or Pull requests regarding .NET code PR: main and removed kernel Issues or pull requests impacting the core kernel labels Apr 20, 2023
@MovGP0

Copy link
Copy Markdown

Instead of

string resultString;
if (countInt == 1)
{
    resultString = results.FirstOrDefault() ?? string.Empty;
}
else
{
    resultString = JsonSerializer.Serialize(results);
}

return resultString;

you could do something like

return countInt == 1
    ? results.FirstOrDefault() ?? string.Empty
    : JsonSerializer.Serialize(results);

@lemillermicrosoft Lee Miller (lemillermicrosoft) added kernel Issues or pull requests impacting the core kernel and removed PR: main labels Apr 21, 2023
@github-actions github-actions Bot added kernel.core and removed kernel Issues or pull requests impacting the core kernel labels Apr 23, 2023
@adrianwyatt

Copy link
Copy Markdown
Contributor

LGTM - please resolve conflicts and we'll bring it in.

@adrianwyatt Adrian Bonar (adrianwyatt) added PR: ready to merge PR has been approved by all reviewers, and is ready to merge. and removed PR: feedback to address Waiting for PR owner to address comments/questions kernel.core labels Apr 25, 2023
@lemillermicrosoft Lee Miller (lemillermicrosoft) added the kernel Issues or pull requests impacting the core kernel label Apr 26, 2023
@lemillermicrosoft Lee Miller (lemillermicrosoft) added kernel.core PR: feedback to address Waiting for PR owner to address comments/questions labels Apr 26, 2023
@adrianwyatt Adrian Bonar (adrianwyatt) removed the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Apr 26, 2023
@shawncal

Copy link
Copy Markdown
Contributor

Merged from main, updated Google connector too, as common interface changes require it.

@shawncal
Shawn Callegari (shawncal) dismissed Devis Lucato (dluc)’s stale review May 6, 2023 05:02

Request for change description fulfilled.

@Tbablani

Tbablani commented May 8, 2023

Copy link
Copy Markdown

Do you have new nuget package for this fix. I downloaded the code locally and trying to run it from a console app adding local references and it is giving me build code.

@Tbablani

Tbablani commented May 8, 2023

Copy link
Copy Markdown

trying to test this through a console app. As latest Nuget is not available for this issue.
Adding local project references to the DLL. It is not working . Getting errors when trying to run it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Issues or pull requests impacting the core kernel .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.

7 participants