Skip to content

Conversation

@Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Sep 6, 2023

Fixes #30239


Internal previews

📄 File 🔗 Preview link
aspnetcore/fundamentals/logging/index.md Logging in .NET Core and ASP.NET Core

Comment on lines 73 to 79
// </snippet4>
var app = builder.Build();

app.MapGet("/", () => "Hello World!");

app.Run();
// </snippet4>
Copy link
Contributor

Choose a reason for hiding this comment

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

there are two snippet4 end tags

@Rick-Anderson Rick-Anderson marked this pull request as ready for review September 6, 2023 21:45

`ILogger<T>` is equivalent to calling `CreateLogger` with the fully qualified type name of `T`.

***NOTE:*** [LoggerFactory.Create](/dotnet/api/microsoft.extensions.logging.loggerfactory.create) Creates a new instance of <xref:Microsoft.Extensions.Logging.ILoggerFactory> that's configured using the provided `configure` delegate. The new instance of `ILoggerFactory` creates a new logging pipeline, different from the default logging pipeline. The new logging pipeline is not configured by the `Logging` section of `appsettings.json` or `appsettings.{ENVIRONMENT}.json`. For more information, see [Logging configuration](xref:fundamentals/logging/index#configure-logging).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noahfalk @cijothomas who can review this NOTE: and suggest more accurate wording. Much of it was written by the ChatGPT plugin for VS.

Copy link
Member

Choose a reason for hiding this comment

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

The text of the note seems quite accurate to me, but the note itself feels disconnected from the text around it. Assuming that we make the other changes in the snippets then this docs page will have no references to using LoggerFactory.Create and I think the note can be omitted. I am guessing a lot of the confusion was occurring because some sample snippets were using LoggerFactory.Create and people would copy those snippets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming that we make the other changes in the snippets then this docs page will have no references to using LoggerFactory.Create and I think the note can be omitted.

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

(Very impressed with ChatGPT's notes, btw!)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Rick-Anderson I suggest to remove this note. Its no longer needed, and having it there is probably confusing without any context.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

I suggested a few fixes, otherwise looks good 👍

@Rick-Anderson Rick-Anderson merged commit 62cf0d4 into main Sep 7, 2023
@Rick-Anderson Rick-Anderson deleted the remove.log.create branch September 7, 2023 22:29
@Rick-Anderson Rick-Anderson mentioned this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid usage of LoggerFactory.Create to setup logging pipeline

5 participants