search

nreco / logging

Generic file logger for .NET Core (FileLoggerProvider) with minimal dependencies
MIT License
284 stars 56 forks source link

BC: Fix: #61 NReco's FileLoggerExtensions should NOT be in the Micros… #62

Closed jwdonahue closed 8 months ago

jwdonahue commented 8 months ago

This fixes #61. Updated xunit, disabled xml doc generation (causing 14 warnings), and added AssemblyInfo.cs file in order to suppress an xunit warning that seems safe to ignore for now.

jwdonahue commented 8 months ago

The namespace change did not break the unit tests, but I still added the "BC" tag to the commit message to be cautious.

jwdonahue commented 8 months ago

Forgot to clean up all the useless using statements...

VitaliyMF commented 8 months ago

I agree that FileLoggerExtensions should not be in Microsoft.Extensions.Logging and this should be the only change (one line of code) in this PR.

It makes sense to use a separate PR for xUnit tests (NReco.Logging.Tests) and formatting changes in NReco.Logging.File - and in fact it is better to discuss them. In this lib style conventions are:

Also I don't think these changes in NReco.Logging.File.csproj are really needed:

    <PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Logging.Configuration" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" Version="8.0.0" />

This forces all users of this lib to upgrade to the latest Microsoft.Extensions.Logging v8 which may be undesired or even not possible. For backward compatibility, I think it makes sense to keep dependencies on version 3.1.*

If referencing old version of Microsoft.Extensions.Logging causes a problem this should be reported as an issue first.

jwdonahue commented 8 months ago

Except I could not test anything without making those other changes, so if you want a one liner fix, you're on your own.

jwdonahue commented 8 months ago

This library isn't worth the effort i already put into it, so I am dropping this PR.

jwdonahue commented 8 months ago

Now the caffeine is kicking in this morning, I just remembered I am setup with the latest preview SDK, so not really in a position to easily and safely make any contributions to older projects at the moment.

VitaliyMF commented 8 months ago

I appreciate your wiliness to contribute, and thank you for noticing about extension method namespace (I cannot remember why it was placed in 'Microsoft' namespace - really weird, maybe just typos). I'll do this one-liner change by myself, no problems with that.

It makes sense to upgrade xunit test to, at least, net6 target too (which is widely used at this moment - I'm sure that most projects are not migrated to net8 yet). Will do that too.

Regarding other changes, I don't like PRs with a lot of changed lines that are simply re-formatting or removing unused namespaces. Not a bad thing by itself, however these changes doesn't change anything (and make git 'blame' less usable).

My top priority in maintaining these libs is backward compatibility; this means that I don't accept PRs that simply may break it without really strong rationale. It makes sense

1) to discuss changes first before putting time into PR 2) to change minimum lines of code to solve concrete issue (occam's razor principle), this helps a lot to review & accept the PR. This is an open source lib and I also maintain it in my free time; I'm not obligated to spend a lot of time for processing PRs. I'm sure that your time is expensive, but mine is too.

jwdonahue commented 8 months ago

Agreed. I forget I was operating in a bleeding edge environment. When I could not even run the unit tests, I just pushed through and made it work. Apologies for that. Hope the resolution of the namespace issue doesn't break too many of your customers.

I was really just looking for a simple example of MEL file logging I could use as a template for a JSON file logger, and I need to be able to target .NET6..8.x. Haven't bothered to check my work against earlier versions of VS2022 or other frameworks. I suspect that VS2022 will be old hat by the time I complete my work, so not going back at this point.

Thank you for your contribution to OSS and helping to fill the file logging gap in MEL.

VitaliyMF commented 8 months ago

@jwdonahue I've just made necessary lib upgrade, now you should be able to run tests/example with the latest SDK (if you have only 'net8' without 'net6' at all, simply change target framework from 'net6.0' to 'net8.0', no other changes are needed -- I checked that with SDK 8.0.100.

Now nuget has a separate targets for net6 and net8 to reference appropriate 'Microsoft.Extensions.Logging' version, and all legacy targets are still supported with generic 'netstandard2.0', so nuget package should remain fully backward compatible.

Thank you for taking my attention to these issues, really appreciate that. I hope you'll able to use NReco.Logging.File now. Merry Christmas btw :)