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

Deprecate Logging Facility's LoggerImplementation enum #336

Merged
merged 1 commit into from
Sep 17, 2017

Conversation

jonorossi
Copy link
Member

This deprecation also includes UseLog4Net, UseNLog and the loggingApi
XML property. This is designed to simplify the logging facility to
remove something it shouldn't have to care about, consumers should
directly use LogUsing and specify the factory from Castle Core or
elsewhere. These deprecated members will be removed in the next major
release.

Relates to #327.

This deprecation also includes UseLog4Net, UseNLog and the loggingApi
XML property. This is designed to simplify the logging facility to
remove something it shouldn't have to care about, consumers should
directly use LogUsing<T> and specify the factory from Castle Core or
elsewhere. These deprecated members will be removed in the next major
release.
@ghost
Copy link

ghost commented Sep 13, 2017

Will review this in detail tomorrow.

- Ignore minor/patch level version for AssemblyVersionAttribute as this creates binding errors for downstream libraries (@fir3pho3nixx, Core/#288)
- Fixing master build to use Castle.Core(loggers et al) 4.1.1 (@fir3pho3nixx, #321, #326)
- Fix binding errors because assembly version had too much detail, assembly version is now x.0.0.0 (@fir3pho3nixx, #329)
- Update Castle Core to 4.1.1 (TODO: update again) to resolve assembly version problems because Castle Core also had too much detail
Copy link

Choose a reason for hiding this comment

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

Can I just update this one for us, what version are we aiming for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure right this second, but we'll get to it.

/// </summary>
[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

@ghost ghost Sep 14, 2017

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had no intention of bumping the version to 5.0 just for this minor change, hence marking it obsolete. I agree there is a lot of stuff marked obsolete for a long time and it is time we remove much of it, I've logged an issue so we can look at doing this for Windsor 5.

@@ -74,6 +76,7 @@ public LoggingFacility()
/// Initializes a new instance of the <see cref="LoggingFacility" /> class.
/// </summary>
/// <param name="loggingApi"> The LoggerImplementation that should be used </param>
[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

@@ -83,6 +86,7 @@ public LoggingFacility(LoggerImplementation loggingApi) : this(loggingApi, null)
/// </summary>
/// <param name="loggingApi"> The LoggerImplementation that should be used </param>
/// <param name="configFile"> The configuration file that should be used by the chosen LoggerImplementation </param>
[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

@@ -103,34 +107,38 @@ public LoggingFacility(string customLoggerFactory, string configFile)
/// <param name="loggingApi"> The LoggerImplementation that should be used </param>
/// <param name="configFile"> The configuration file that should be used by the chosen LoggerImplementation </param>
/// <param name="customLoggerFactory"> The type name of the type of the custom logger factory. (only used when loggingApi is set to LoggerImplementation.Custom) </param>
[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

@@ -165,21 +170,25 @@ public LoggingFacility ToLog(string name)
}

#if CASTLE_SERVICES_LOGGING
[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

public LoggingFacility UseLog4Net()
{
return LogUsing(LoggerImplementation.Log4net);
}

[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

public LoggingFacility UseLog4Net(string configFile)
{
return UseLog4Net().WithConfig(configFile);
}

[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

public LoggingFacility UseNLog()
{
return LogUsing(LoggerImplementation.NLog);
}

[Obsolete("A logger factory implementation type should be provided via LogUsing<T>(), this will be removed in the future.")]
Copy link

Choose a reason for hiding this comment

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

There are currently 131 Obsolete attributes in the code base. Can't we just delete this altogether? Assuming we kick the version up to v5.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Awesome change! I had one question though, would you mind if later on we bring the markdown docs into the solution? This does not have to happen now but it is always something I forget about.

@jonorossi jonorossi merged commit a40cf98 into master Sep 17, 2017
@jonorossi jonorossi deleted the loggerimplemention-enum branch September 17, 2017 14:28
@jonorossi
Copy link
Member Author

Awesome change! I had one question though, would you mind if later on we bring the markdown docs into the solution? This does not have to happen now but it is always something I forget about.

Sounds good to me, I never did it in the past because every file would get listed, but now you can use wildcards.

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.

1 participant