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

Deleting obsolete Castle.Facilities.Logging code, handling FacilityConfig #616

Open
Jevonius opened this issue May 30, 2022 · 3 comments · Fixed by #636
Open

Deleting obsolete Castle.Facilities.Logging code, handling FacilityConfig #616

Jevonius opened this issue May 30, 2022 · 3 comments · Fixed by #636

Comments

@Jevonius
Copy link
Contributor

While looking at adding net6.0 as a target, I took a look at Castle.Facilities.Logging (there is a reference to DiagnosticsLoggerFactory which theoretically can work under net6.0-win but not with the way the code is currently structured). I noticed how much is obsolete in this project, since 2017, so took a look at removing code.

I'm struggling with how to handle ReadLoggingApi:

private LoggerImplementation ReadLoggingApi()
{
if (FacilityConfig != null)
{
var configLoggingApi = FacilityConfig.Attributes["loggingApi"];
if (string.IsNullOrEmpty(configLoggingApi) == false)
{
return converter.PerformConversion<LoggerImplementation>(configLoggingApi);
}
}
return loggerImplementation.GetValueOrDefault(LoggerImplementation.Console);
}

The LoggerImplementation enum is marked obsolete, so looking at FacilityConfig.Attributes["loggingApi"] for the type to create is made more complicated. Any thoughts on how to handle this? Is configuration via FacilityConfig common?

@jonorossi
Copy link
Member

The LoggerImplementation enum was marked obsolete to push people to use LogUsing<T>() rather than loggers being hardcoded in the library.

Without digging into the code and trying to replicate the same change I don't follow what the problems you are facing. Could you summarise your findings and what's a blocker?

@Jevonius
Copy link
Contributor Author

Jevonius commented Oct 4, 2022

The difficulty is in the part around reading FacilityConfig; I haven't really used this, so I'm not sure the use cases for it and whether it still needs to be supported, or to what level.
The code above is using the loggingApi configuration attribute to work out which LoggerImplementation to return, but that enum is marked obsolete.

My proposal, having had another read through the code, is that if FacilityConfig is used instead of LogUsing<T>(), it will be via the use of the FacilityConfig.Attributes["customLoggerFactory"] value - FacilityConfig.Attributes["loggingApi"] will no longer be available.

Does this seem reasonable?

@jonorossi
Copy link
Member

Reopening after being automatically closed by #636. This one was about removing old code using FacilityConfig, not sure exactly, but reopen.

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 a pull request may close this issue.

2 participants