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

Added NullLoggerFactory to Logging.Abstractions #568

Merged
merged 1 commit into from
Mar 10, 2017

Conversation

ajaybhargavb
Copy link
Contributor

Issue - #566
Removing NullLogger* types from Logging.Testing will break other repos. That should be done separately.

@Eilon @muratg

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM


namespace Microsoft.Extensions.Logging.Abstractions
{
public class NullLoggerFactory : ILoggerFactory
Copy link
Member

Choose a reason for hiding this comment

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

Doc comments plz kthxbye!

@ajaybhargavb
Copy link
Contributor Author

🆙 📅

@@ -3,15 +3,20 @@

namespace Microsoft.Extensions.Logging.Abstractions
{
/// <summary>
/// An <see cref="ILoggerFactory"/> used to create instance of <see cref="NullLogger"/>.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding some text that explicitly states that this logger never logs anything? Same goes for the methods below...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It says that in the docs for NullLogger. Do we have to say that here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it doesn't hurt to add it here. Will update

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/null-logger branch from 60f329c to 3807733 Compare March 9, 2017 20:35
@ajaybhargavb
Copy link
Contributor Author

🆙 📅


/// <inheritdoc />
/// <remarks>
/// This method explicitly no ops.
Copy link
Member

Choose a reason for hiding this comment

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

I think this phrasing can be improved, no ops is nerd-speak. How about "This method ignores the parameter and does nothing."


/// <inheritdoc />
/// <remarks>
/// This returns a <see cref="NullLogger"/> instance which explicitly logs nothing.
Copy link
Member

Choose a reason for hiding this comment

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

Remove explicitly.

@ajaybhargavb
Copy link
Contributor Author

🆙 📅

@davidfowl
Copy link
Member

davidfowl commented Mar 10, 2017

👏 That only took 8 hours :trollface:

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/null-logger branch from 61a228f to b4ecccd Compare March 10, 2017 01:01
@ajaybhargavb ajaybhargavb merged commit b4ecccd into dev Mar 10, 2017
@ajaybhargavb ajaybhargavb deleted the ajbaaska/null-logger branch March 10, 2017 01:02
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