-
Notifications
You must be signed in to change notification settings - Fork 6k
Add console logger breaking changes #19739
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
Conversation
We may want to wait for the Preview 8 APIs to be published and verify that the xref warning goes away: • Line 38, Column 41: [Warning] Cross reference not found: 'Microsoft.Extensions.Logging.Console.ConsoleLoggerOptions.FormatterName'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from the xref not resolving.
- <xref:Microsoft.Extensions.Logging.Console.ConsoleLoggerOptions.DisableColors?displayProperty=fullName> | ||
- <xref:Microsoft.Extensions.Logging.Console.ConsoleLoggerOptions.IncludeScopes?displayProperty=fullName> | ||
- <xref:Microsoft.Extensions.Logging.Console.ConsoleLoggerOptions.TimestampFormat?displayProperty=fullName> | ||
- `UseUtcTimestamp` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this type?
Googling it gives no results, and it's the only one without an xref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching in runtime repo, it's a public property defined here. But why it doesn't exist in API reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting...thanks for finding that. It looks like the property was introduced last October. I'll check if it's in the binary that we created the API docs from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something's not right here. @safern the Microsoft.Extensions.Logging.Console.dll file is listed under the microsoft.aspnetcore.app folder in the preview 7 assemblies that you sent me. We do not include the assemblies in that folder when generating the API docs. What needs to be changed here? /cc @BillWagner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why they're under the Microsoft.AspNetCore.App
folder is because that ships as part of the ASP.NET shared framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the list I use... whatever doesn't start with Microsoft.AspNetCore
is part of Platform Extensions.
https://github.com/dotnet/api-catalog-infra/blob/feb11538b8b19972da8e79d0dbfbee52cb2d34c3/eng/droplayout.props#L63-L104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safern Your list doesn't match up with Sourabh's at all. Also, it doesn't include the Microsoft.Extensions.Logging.Console assembly that's the subject of this missing API. I will plan to import all the APIs from all the assemblies that moved from dotnet/extensions to dotnet/runtime:
- Microsoft.Extensions.Caching.Abstractions
- Microsoft.Extensions.Caching.Memory
- Microsoft.Extensions.Configuration
- Microsoft.Extensions.Configuration.Abstractions
- Microsoft.Extensions.Configuration.Binder
- Microsoft.Extensions.Configuration.CommandLine
- Microsoft.Extensions.Configuration.EnvironmentVariables
- Microsoft.Extensions.Configuration.FileExtensions
- Microsoft.Extensions.Configuration.Ini
- Microsoft.Extensions.Configuration.Json
- Microsoft.Extensions.Configuration.UserSecrets
- Microsoft.Extensions.Configuration.Xml
- Microsoft.Extensions.DependencyInjection
- Microsoft.Extensions.DependencyInjection.Abstractions
- Microsoft.Extensions.FileProviders.Abstractions
- Microsoft.Extensions.FileProviders.Composite
- Microsoft.Extensions.FileProviders.Physical
- Microsoft.Extensions.FileSystemGlobbing
- Microsoft.Extensions.Hosting
- Microsoft.Extensions.Hosting.Abstractions
- Microsoft.Extensions.Http
- Microsoft.Extensions.Logging
- Microsoft.Extensions.Logging.Abstractions
- Microsoft.Extensions.Logging.Configuration
- Microsoft.Extensions.Logging.Console
- Microsoft.Extensions.Logging.Debug
- Microsoft.Extensions.Logging.EventLog
- Microsoft.Extensions.Logging.EventSource
- Microsoft.Extensions.Logging.Testing
- Microsoft.Extensions.Logging.TraceSource
- Microsoft.Extensions.Options
- Microsoft.Extensions.Options.ConfigurationExtensions
- Microsoft.Extensions.Options.DataAnnotations
- Microsoft.Extensions.Primitives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My list doesn't match just list because my list doesn't contain the Microsoft.Extensions packages that ship as part of Microsoft.AspNetCore.App package.
It didn't make sense to include those packages as part of both Platform Extensions and aspnetcore.
I can double check though, but I'm pretty sure, cause at the beginning we started including all Microsoft.Extensions.* packages into Platform Extensions and I figured there were duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we can do so that they appear as part of the docs is to include all the packages that are produced in runtime (Microsoft.Extensions.*) into the platform extensions drop if that's what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we can do so that they appear as part of the docs is to include all the packages that are produced in runtime (Microsoft.Extensions.*) into the platform extensions drop if that's what we want.
Yes, it sounds to me like that would work going forward. I'm a noob here though.
includes/core-changes/corefx/5.0/obsolete-consoleloggeroptions-properties.md
Show resolved
Hide resolved
includes/core-changes/corefx/5.0/obsolete-consoleloggeroptions-properties.md
Outdated
Show resolved
Hide resolved
includes/core-changes/corefx/5.0/obsolete-consoleloggeroptions-properties.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from posted comments, LGTM
@gewarren This approved PR is getting stale. Are you going to merge it, or are you waiting on something? |
@BillWagner I was waiting on the Preview 8 APIs so it didn't introduce build warnings. I'm still seeing some warnings, so I'm looking at those now. |
Fixes #19533.
Preview link.