Skip to content
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

Remove ConfigurationBinder usage from Console Logging #82098

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

eerhardt
Copy link
Member

This allows ConfigurationBinder, and its dependencies like TypeConverter, to be trimmed in an application that uses Console Logging, like an ASP.NET API application.

Fix #81931

This allows ConfigurationBinder, and its dependencies like TypeConverter,
to be trimmed in an application that uses Console Logging, like an
ASP.NET API application.

Fix dotnet#81931
@eerhardt eerhardt added area-Extensions-Logging size-reduction Issues impacting final app size primary for size sensitive workloads labels Feb 14, 2023
@eerhardt eerhardt requested review from layomia and tarekgh February 14, 2023 16:30
@eerhardt eerhardt self-assigned this Feb 14, 2023
@ghost
Copy link

ghost commented Feb 14, 2023

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

This allows ConfigurationBinder, and its dependencies like TypeConverter, to be trimmed in an application that uses Console Logging, like an ASP.NET API application.

Fix #81931

Author: eerhardt
Assignees: eerhardt
Labels:

area-Extensions-Logging, size-reduction

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Feb 14, 2023

Changes LGTM. do we have tests when having invalid values inside the configuration? something when defining string with null value (which we convert it to empty string. or when setting bool value to something different than true or false. Or having wrong int values. I am asking to ensure the behavior does not change when encountering such cases.

@layomia
Copy link
Contributor

layomia commented Feb 15, 2023

Overall LGTM.

@tarekgh
Copy link
Member

tarekgh commented Feb 16, 2023

LGTM!

@eerhardt eerhardt merged commit 0f129e4 into dotnet:main Feb 16, 2023
@eerhardt eerhardt deleted the RemoveConfigBinderFromConsole branch February 16, 2023 03:05
@ericstj
Copy link
Member

ericstj commented Mar 16, 2023

@eerhardt would we just replace this with the Configuration Binder source generator generated code once that's ready?

@eerhardt
Copy link
Member Author

@eerhardt would we just replace this with the Configuration Binder source generator generated code once that's ready?

Yes, that is the goal/intention. See #81931 (comment). If/when the Configuration Binder source generator can support this library, we should switch to using it.

Things that may be tricky:

  • netstandard2.0 and netfx support
  • Properties that are annotated with ObsoleteAttribute still need to be bound
  • The mutable struct JsonWriterOptions needs to be supported

@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MS.Extensions.Logging.Console's dependency on ConfigurationBinder
5 participants