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

ILogger support #82

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

ILogger support #82

wants to merge 10 commits into from

Conversation

askids
Copy link

@askids askids commented Aug 13, 2022

Replace console.Write logging with ILogger using Microsoft logging extensions.

Retain only .Net Standard 2.0 and 2.1 support. Bump version to indicate that its a major change.
Added support to use iLogger instead of writing log using console.Write. Updated example
Reflect support for only .Net standard 2.0 and 2.1
Updated to build/test for only .Net standard 2.0 and 2.1
@askids
Copy link
Author

askids commented Aug 13, 2022

#76 - related to this question.

@StefH StefH self-requested a review August 13, 2022 08:39
Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

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

Can you please take look at my comments?

@@ -6,7 +6,7 @@
<Authors>Jakub Maly;Stef Heyenrath;NTTAKR</Authors>
<Copyright>Copyright Jakub Maly &amp; Stef Heyenrath © 2018-2022</Copyright>
<Company>Jakub Maly</Company>
<TargetFrameworks>net20;net35;net40;net45;net452;netstandard1.3;netstandard2.0;netstandard2.1</TargetFrameworks>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please bring these older frameworks back.

Copy link
Author

Choose a reason for hiding this comment

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

None of these older frameworks are supported. If someone needs to use this library with older frameworks, they can continue using the older version 3.0.x. Since we are using Microsoft.Extensions.Logging.Abstractions, this will force us to support 4.6.1 and above only. That was the reason to retain only .netstandard 2.0 and 2.1 and bump the version to 3.1.x.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@askids
As general remark for NuGet packages, you don't know who is using this library which what framework, so even if frameworks are old, you cannot just remove these.

To solve this, you can make an own interface ILogger + ILogger<T> which mimics the Microsoft.Extensions.Logging.Abstractions for the older frameworks.
With this solution, you can use Microsoft.Extensions.Logging.Abstractions for .netstandard 2.0 and higher, and just use the custom ILogger for the older versions.

And a default implementation for a ConsoleLogger is required to keep the behavior and usage the same without having to use a new constructor.

Copy link
Author

Choose a reason for hiding this comment

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

As general remark for NuGet packages, you don't know who is using this library which what framework, so even if frameworks are old, you cannot just remove these.

I get that. But I don't see this as a version that is continuing the support for old consumers or adding feature to it. This version is meant to be for those people who are upgrading/transitioning to the new model. Adding support for namesake for the old version is not going to help any existing consumers solve any new problem. Why would they even want to upgrade to this version which is not adding any new features for them or fixing any issues? If you want, we can bump the version from 3.1 to 4.0 to show that its a breaking change and update the documentation accordingly.

ReleaseNotes.md Outdated Show resolved Hide resolved
src/CommandLineArgumentsParser/CommandLineParser.cs Outdated Show resolved Hide resolved
src/CommandLineArgumentsParser/Messages.cs Show resolved Hide resolved
Remove Microsoft.Extensions.Logging dependency.
@askids
Copy link
Author

askids commented Aug 13, 2022

Can you please take look at my comments?
Replied to all individual comments. Please check.

Add default Console Logger
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.

2 participants