Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Obsolete old APIs #605

Merged
merged 1 commit into from
May 3, 2017
Merged

Obsolete old APIs #605

merged 1 commit into from
May 3, 2017

Conversation

BrennanConroy
Copy link
Member

No description provided.

@cwe1ss
Copy link
Contributor

cwe1ss commented Apr 24, 2017

Awesome, it's great to see this!

Small nitpick: There's a Visual Studio Version downgrade in the sln file.

@@ -19,9 +19,15 @@ public interface ILoggerFactory : IDisposable
ILogger CreateLogger(string categoryName);

/// <summary>
/// <para>
/// This method is being removed from the ILoggerFactory abstraction, to have similar behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend following a pattern closer to the guidelines: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes

/// Adds an <see cref="ILoggerProvider"/> to the logging system.
/// </summary>
/// <param name="provider">The <see cref="ILoggerProvider"/>.</param>
[Obsolete("This method is being removed from the ILoggerFactory abstraction, to have similar behavior" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -19,9 +19,13 @@ public interface ILoggerFactory : IDisposable
ILogger CreateLogger(string categoryName);

/// <summary>
/// <para>
/// This method is obsolete, use AddProvider on the concrete Microsoft.Extensions.Logging.LoggerFactory class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we follow the guidelines on Recommend following a pattern closer to the guidelines: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes a little more closely? I recommend using the exact phrasing in there, i.e. This method is obsolete and will be removed in a future version. The recommended alternative is Microsoft.Extensions.Logging.LoggerFactory.AddProvider().

Copy link
Contributor

Choose a reason for hiding this comment

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

(Applies to all the obsoletions in this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is obsolete and will be removed in a future version. The recommended alternative is Microsoft.Extensions.Logging.AddConsole() on Microsoft.Extensions.Logging.LoggerFactory.

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is obsolete and will be removed in a future version. The recommended alternative is to call the Microsoft.Extensions.Logging.AddConsole() extension method on the Microsoft.Extensions.Logging.LoggerFactory instance.

@BrennanConroy BrennanConroy added this to the 2.0.0-preview1 milestone Apr 26, 2017
Copy link
Contributor

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Looks good! Can you log a new bug that says to actually delete all these obsolete methods in a future version?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants