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 obsolete code in Castle.Facilities.Logging #636

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

Jevonius
Copy link
Contributor

@Jevonius Jevonius commented Oct 5, 2022

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.

@jonorossi jonorossi marked this pull request as draft November 30, 2022 00:15
@Jevonius Jevonius marked this pull request as ready for review November 30, 2022 18:51
Copy link
Member

@jonorossi jonorossi left a 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.

🎉

@jonorossi jonorossi added this to the v6.0.0 milestone Dec 2, 2022
@@ -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)
Copy link
Member

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.

Copy link

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.
@Jevonius
Copy link
Contributor Author

I've re-based this, and build now passing again. Still need to update the docs regarding deprecated APIs.

@Regenhardt
Copy link

Looks like this is only missing the changelog update now. @Jevonius would you accept those as a PR into your feature branch? I think @lunchin did the hard work already so I'd happily add that to the changelog so we get get #621 out the door.

@thibnes
Copy link

thibnes commented Jul 7, 2023

@jonorossi is there something preventing us from moving forward here as suggested by @Regenhardt?

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.

Deleting obsolete Castle.Facilities.Logging code, handling FacilityConfig
5 participants