-
Notifications
You must be signed in to change notification settings - Fork 458
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 obsolete code in Castle.Facilities.Logging
#636
Remove obsolete code in Castle.Facilities.Logging
#636
Conversation
657a57a
to
7bf729f
Compare
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.
Holy crap, YES YES YES!!! I can't believe it was 5 years ago I deprecated these. Really glad to see that code finally go, it caused so much pain during the early .NET Core migration days.
🎉
src/Castle.Facilities.AspNet.SystemWeb.Tests/Castle.Facilities.AspNet.SystemWeb.Tests.csproj
Show resolved
Hide resolved
@@ -12,6 +12,7 @@ Bugfixes: | |||
|
|||
Breaking Changes: | |||
- Microsoft.Extensions.Hosting related methods have been removed from the Castle.Windsor.Extensions.DependencyInjection package to the Castle.Windsor.Extensions.Hosting package (@ikkentim, #625, #628) | |||
- Obsolete components in Castle.Facilities.Logging have been removed. Extensions methods for built-in logging factories have been added. (@Jevonius, #616) |
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.
Could you list out the public APIs that have been removed like we do in Castle Core (https://github.com/castleproject/Core/blob/master/CHANGELOG.md#440-2019-04-05) so it makes it clear to upgraders and to enable search. You don't need every enum member, just the enum type name.
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.
Here are the removed classes and methods. Is anything else needed to merge this and create a release. I am happy to help get this out the door.
- Removed class Castle.Facilities.Logging.LoggerImplementation
- Removed constructor Castle.Facilities.Logging.LoggingFacility(LoggerImplementation loggingApi)
- Removed constructor Castle.Facilities.Logging.LoggingFacility(LoggerImplementation loggingApi, string configFile)
- Removed constructor Castle.Facilities.Logging.LoggingFacility(string customLoggerFactory, string configFile)
- Removed method Castle.Facilities.Logging.LoggingFacility.LogUsing(LoggerImplementation loggingApi)
- Removed method Castle.Facilities.Logging.LoggingFacility.UseLog4Net()
- Removed method Castle.Facilities.Logging.LoggingFacility.UseLog4Net(string configFile)
- Removed method Castle.Facilities.Logging.LoggingFacility.UseNLog()
- Removed method Castle.Facilities.Logging.LoggingFacility.UseNLog(string configFile)
…on symbols as they're no longer required The tests now work across all target frameworks.
This works around an issue with NUnit3TestAdapter private referencing this version, stopping assembly binding redirects, and Windsor struggling to call `GetExportedTypes` as a result.
0e494b3
to
f021840
Compare
I've re-based this, and build now passing again. Still need to update the docs regarding deprecated APIs. |
@jonorossi is there something preventing us from moving forward here as suggested by @Regenhardt? |
This is the work to resolve #616; it's built off the #630 branch, so I anticipate rebasing this branch once that's merged down. There will be more noise in this PR until then.