Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change UseLogging to accept an ILoggerFactory instead of ILogger #5682

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 20, 2024

Fits better with DI, and makes it consistent with UseFunctionInvocation and UseOpenTelemetry.

Microsoft Reviewers: Open in CodeFlow

Fits better with DI, and makes it consistent with UseFunctionInvocation and UseOpenTelemetry.
@stephentoub stephentoub merged commit 042b4e6 into dotnet:main Nov 20, 2024
6 checks passed
@stephentoub stephentoub deleted the useloggingfactory branch November 20, 2024 19:57
stephentoub added a commit to stephentoub/extensions that referenced this pull request Nov 20, 2024
…net#5682)

Fits better with DI, and makes it consistent with UseFunctionInvocation and UseOpenTelemetry.
@stephentoub stephentoub requested a review from Copilot December 17, 2024 00:38

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Microsoft.Extensions.AI.Abstractions.Tests.csproj: Language not supported
  • test/Libraries/Microsoft.Extensions.AI.Integration.Tests/Microsoft.Extensions.AI.Integration.Tests.csproj: Language not supported
  • test/Libraries/Microsoft.Extensions.AI.Tests/Microsoft.Extensions.AI.Tests.csproj: Language not supported
  • test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/CapturingLogger.cs: Evaluated as low risk
  • test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/LoggingChatClientTests.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)

src/Libraries/Microsoft.Extensions.AI/ChatCompletion/LoggingChatClientBuilderExtensions.cs:23

  • The parameter name 'logger' should be updated to 'loggerFactory' to reflect the change in type and improve clarity.
this ChatClientBuilder builder, ILogger? logger = null, Action<LoggingChatClient>? configure = null)

src/Libraries/Microsoft.Extensions.AI/Embeddings/LoggingEmbeddingGeneratorBuilderExtensions.cs:20

  • The phrase 'a required instance' is unclear. Consider rephrasing to 'an instance will be resolved from the service provider' to improve clarity.
If not supplied, a required instance will be resolved from the service provider.

test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs:416

  • The variable name 'collector' should be consistent with the naming convention used in other methods, such as 'logCollector'.
var collector = new FakeLogCollector();

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants