KEMBAR78
Remove ConfigurationBinder usage from Console Logging by eerhardt · Pull Request #82098 · dotnet/runtime · GitHub
Skip to content

Remove ConfigurationBinder usage from Console Logging #82098

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 16, 2023
Merged

Remove ConfigurationBinder usage from Console Logging #82098

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

@eerhardt
Remove ConfigurationBinder usage from Console Logging
583731d 
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: -

@build-analysis build-analysis bot mentioned this pull request Feb 14, 2023
Closed
@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
@eerhardt eerhardt mentioned this pull request Mar 14, 2023
Open
@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

@layomia layomia mentioned this pull request Mar 17, 2023
Closed
@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.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@tarekgh tarekgh tarekgh approved these changes

@layomia layomia Awaiting requested review from layomia

Assignees

@eerhardt eerhardt

Labels

area-Extensions-Logging size-reduction Issues impacting final app size primary for size sensitive workloads

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@eerhardt @tarekgh @layomia @ericstj @stephentoub