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

Change logging style #3387

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Change logging style #3387

merged 1 commit into from
Oct 30, 2015

Conversation

ryanbrandenburg
Copy link
Contributor

Move existing instances of logging to the new style.

@dnfclas
Copy link

dnfclas commented Oct 22, 2015

Hi @ryanbrandenburg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch from c77d5a7 to 74504c2 Compare October 22, 2015 17:23
rynowak
rynowak reviewed Oct 22, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.Core/Controllers/FilterActionInvoker.cs
@@ -297,7 +287,7 @@ private Task InvokeAllAuthorizationFiltersAsync()
}
else
{
Logger.LogWarning(AuthorizationFailureLogMessage, current.FilterAsync.GetType().FullName);
Logger.AuthorizationFailure(current.FilterAsync.GetType().FullName);
Copy link
Member

Choose a reason for hiding this comment

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

Pass in the filter, and do the deep dive into inside your extension method

Copy link
Member

Choose a reason for hiding this comment

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

Repeat ad nauseum

@rynowak
Copy link
Member

rynowak commented Oct 23, 2015

overall looking good, a few things here can be simplified.

@rynowak
Copy link
Member

rynowak commented Oct 23, 2015

⌚

rynowak
rynowak reviewed Oct 23, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.TagHelpers/Internal/ModeMatchResult.cs
@@ -63,14 +64,13 @@ public class ModeMatchResult<TMode>
match => match.PresentAttributes.Any(
attribute => PartiallyMatchedAttributes.Contains(
attribute, StringComparer.OrdinalIgnoreCase)));

//TODO: What the heck is this thing?
logger.LogWarning(new PartialModeMatchLogValues<TMode>(uniqueId, viewPath, partialOnlyMatches));
Copy link
Member

Choose a reason for hiding this comment

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

We still want to create the extension method here, just move PartialModeMatchLogValues to be a private class inside that class, and then use LogWarning as-is.

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch 2 times, most recently from 6a6849b to 9cff708 Compare October 26, 2015 17:45
NTaylorMullen
NTaylorMullen reviewed Oct 26, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.Core/Logging/DefaultActionSelectorLoggerExtensions.cs
{
public static class DefaultActionSelectorLoggerExtensions
{
private static Action<ILogger, string, Exception> _ambiguousActions;
Copy link
Contributor

Choose a reason for hiding this comment

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

readonly on both.

@NTaylorMullen
Copy link
Contributor

Left a few guiding comments in DefaultActionSelectorLoggerExtensions, apply the changes to other files where necessary.

NTaylorMullen
NTaylorMullen reviewed Oct 26, 2015
View reviewed changes
src/Microsoft.AspNet.Mvc.Core/Logging/DefaultActionSelectorLoggerExtensions.cs
"constraint '{ActionConstraint}'");
}

public static void AmbiguousActions(this ILogger logger, string actionNames)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we had [NotNull] we'd typically decorate the heck out of all of our public extension methods. Now that we don't, with this new pattern are we not doing manual null checks? I'm conflicted about it myself which is why i ask because it'd add a to of boiler plate. RIP [NotNull]

Same comment applies below.

Copy link
Member

Choose a reason for hiding this comment

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

This should all be internal

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@NTaylorMullen
Copy link
Contributor

⌚

@NTaylorMullen
Copy link
Contributor

:shipit:

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch 4 times, most recently from 6ce0145 to 77fbc44 Compare October 29, 2015 21:56
@rynowak
Copy link
Member

rynowak commented Oct 30, 2015

Is this still waiting to be merged?

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch from 77fbc44 to 0561f4c Compare October 30, 2015 16:21
@ryanbrandenburg
Copy link
Contributor Author

I keep having CI failures after the rebase, I'll make it happen.

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch from 57d7eb1 to 18a5032 Compare October 30, 2015 17:40
@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/ChangeLoggingStyle branch from 18a5032 to 5763eb5 Compare October 30, 2015 19:47
@ryanbrandenburg ryanbrandenburg merged commit 5763eb5 into dev Oct 30, 2015
@ryanbrandenburg ryanbrandenburg mentioned this pull request Oct 30, 2015
Closed
@ryanbrandenburg ryanbrandenburg deleted the rybrande/ChangeLoggingStyle branch October 30, 2015 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanbrandenburg @dnfclas @rynowak @NTaylorMullen