Skip to content

Commit

Permalink
.Net: Change signature of the TextSearchResult constructor (#9153)
Browse files Browse the repository at this point in the history
### Motivation and Context

- Value is required
- Name & Link are still optional and are removed from the constructor
signature

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
  • Loading branch information
markwallace-microsoft authored Oct 8, 2024
1 parent 6d32d34 commit 805febe
Show file tree
Hide file tree
Showing 12 changed files with 26 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
{
if (result is Hotel hotel)
{
return new TextSearchResult(name: hotel.HotelName, value: hotel.Description, link: $"id://{hotel.HotelId}");
return new TextSearchResult(value: hotel.Description) { Name = hotel.HotelName, Link = $"id://{hotel.HotelId}" };
}
throw new ArgumentException("Invalid result type.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
{
if (result is HotelInfo hotel)
{
return new TextSearchResult(name: hotel.HotelName, value: hotel.Description, link: $"id://{hotel.HotelId}");
return new TextSearchResult(value: hotel.Description) { Name = hotel.HotelName, Link = $"id://{hotel.HotelId}" };
}
throw new ArgumentException("Invalid result type.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
{
if (result is DataModel dataModel)
{
return new TextSearchResult(name: dataModel.Key.ToString(), value: dataModel.Text, link: dataModel.Link);
return new TextSearchResult(value: dataModel.Text) { Name = dataModel.Key.ToString(), Link = dataModel.Link };
}
throw new ArgumentException("Invalid result type.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,9 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
throw new ArgumentException("Result must be a BingWebPage", nameof(result));
}

return new TextSearchResult
return new TextSearchResult(webPage.Snippet?.ToUpperInvariant() ?? string.Empty)
{
Name = webPage.Name?.ToUpperInvariant(),
Value = webPage.Snippet?.ToUpperInvariant(),
Link = webPage.DisplayUrl?.ToUpperInvariant(),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,9 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
throw new ArgumentException("Result must be a Google Result", nameof(result));
}

return new TextSearchResult
return new TextSearchResult(googleResult.Snippet?.ToUpperInvariant() ?? string.Empty)
{
Name = googleResult.Title?.ToUpperInvariant(),
Value = googleResult.Snippet?.ToUpperInvariant(),
Link = googleResult.Link?.ToUpperInvariant(),
};
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/src/Plugins/Plugins.Web/Bing/BingTextSearch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
throw new ArgumentException("Result must be a BingWebPage", nameof(result));
}

return new TextSearchResult(webPage.Name, webPage.Snippet, webPage.Url);
return new TextSearchResult(webPage.Snippet ?? string.Empty) { Name = webPage.Name, Link = webPage.Url };
}
}

Expand Down
2 changes: 1 addition & 1 deletion dotnet/src/Plugins/Plugins.Web/Google/GoogleTextSearch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
throw new ArgumentException("Result must be a Google Result", nameof(result));
}

return new TextSearchResult(googleResult.Title, googleResult.Snippet, googleResult.Link);
return new TextSearchResult(googleResult.Snippet) { Name = googleResult.Title, Link = googleResult.Link };
}
}
#endregion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@ namespace Microsoft.SemanticKernel.Data;
/// - Value associated with the search result
/// - Link reference associated with the search result
/// </remarks>
/// <param name="name">The text search result name.</param>
/// <param name="value">The text search result value.</param>
/// <param name="link">The link reference associated with the text search result.</param>
[Experimental("SKEXP0001")]
public sealed class TextSearchResult(string? name = null, string? value = null, string? link = null)
public sealed class TextSearchResult(string value)
{
/// <summary>
/// The text search result name.
Expand All @@ -26,7 +24,7 @@ public sealed class TextSearchResult(string? name = null, string? value = null,
/// This represents the name associated with the result.
/// If the text search was for a web search engine this would typically be the name of the web page associated with the search result.
/// </remarks>
public string? Name { get; init; } = name;
public string? Name { get; init; }

/// <summary>
/// The link reference associated with the text search result.
Expand All @@ -35,7 +33,7 @@ public sealed class TextSearchResult(string? name = null, string? value = null,
/// This represents a possible link associated with the result.
/// If the text search was for a web search engine this would typically be the URL of the web page associated with the search result.
/// </remarks>
public string? Link { get; init; } = link;
public string? Link { get; init; }

/// <summary>
/// The text search result value.
Expand All @@ -44,5 +42,5 @@ public sealed class TextSearchResult(string? name = null, string? value = null,
/// This represents the text value associated with the result.
/// If the text search was for a web search engine this would typically be the snippet describing the web page associated with the search result.
/// </remarks>
public string? Value { get; init; } = value;
public string Value { get; init; } = value;
}
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,15 @@ private TextSearchResultMapper CreateTextSearchResultMapper()
throw new ArgumentException($"Expected result of type {typeof(TRecord).FullName} but got {result.GetType().FullName}.");
}

return new TextSearchResult(
name: this._propertyReader.Value.GetName(result),
value: this._propertyReader.Value.GetValue(result),
link: this._propertyReader.Value.GetLink(result));
var value = this._propertyReader.Value.GetValue(result) ?? throw new InvalidOperationException($"Value property of {typeof(TRecord).FullName} cannot be null.");
var name = this._propertyReader.Value.GetName(result);
var link = this._propertyReader.Value.GetLink(result);

return new TextSearchResult(value)
{
Name = name,
Link = link,
};
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public void AddVolatileVectorStoreTextSearchWithDelegatesRegistersClass()
this._kernelBuilder.AddVolatileVectorStoreTextSearch<Guid, DataModel>(
"records",
obj => ((DataModel)obj).Text,
obj => new TextSearchResult(name: ((DataModel)obj).Key.ToString(), value: ((DataModel)obj).Text));
obj => new TextSearchResult(value: ((DataModel)obj).Text) { Name = ((DataModel)obj).Key.ToString() });

// Assert.
var kernel = this._kernelBuilder.Build();
Expand Down Expand Up @@ -99,7 +99,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
{
if (result is DataModel dataModel)
{
return new TextSearchResult(name: dataModel.Key.ToString(), value: dataModel.Text);
return new TextSearchResult(value: dataModel.Text) { Name = dataModel.Key.ToString() };
}
throw new ArgumentException("Invalid result type.");
}
Expand Down
4 changes: 3 additions & 1 deletion dotnet/src/SemanticKernel.UnitTests/Data/MockTextSearch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ public Task<KernelSearchResults<object>> GetSearchResultsAsync(string query, Tex
public Task<KernelSearchResults<TextSearchResult>> GetTextSearchResultsAsync(string query, TextSearchOptions? searchOptions = null, CancellationToken cancellationToken = default)
{
int count = searchOptions?.Top ?? this._count;
var results = Enumerable.Range(1, count).Select(i => new TextSearchResult($"Name {i}", $"Result {i}", $"http://example.com/page{i}")).ToList();
var results = Enumerable.Range(1, count).Select(
i => new TextSearchResult($"Result {i}") { Name = $"Name {i}", Link = $"http://example.com/page{i}" })
.ToList();
long? totalCount = searchOptions?.IncludeTotalCount ?? false ? this._totalCount : null;
return Task.FromResult(new KernelSearchResults<TextSearchResult>(results.ToAsyncEnumerable(), totalCount));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public TextSearchResult MapFromResultToTextSearchResult(object result)
{
if (result is DataModel dataModel)
{
return new TextSearchResult(name: dataModel.Key.ToString(), value: dataModel.Text);
return new TextSearchResult(value: dataModel.Text) { Name = dataModel.Key.ToString() };
}
throw new ArgumentException("Invalid result type.");
}
Expand Down

0 comments on commit 805febe

Please sign in to comment.