-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
feature: Add NLog logging adapter #236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks for doing this @dpvreony
src/Splat.NLog/NLogLogger.cs
Outdated
_level = value; | ||
foreach (var configurationLoggingRule in _inner.Factory.Configuration.LoggingRules) | ||
{ | ||
configurationLoggingRule.SetLoggingLevels(RxUitoNLogLevel(value), global::NLog.LogLevel.Fatal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth removing the RxUI reference in this class, just because we in theory trying to make it more "platform' agnostic in the splat package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/Splat.NLog/Helpers.cs
Outdated
/// <remarks> | ||
/// You should configure NLog prior to calling this method. | ||
/// </remarks> | ||
public static void UseNLogWithWrappingFullLogger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth changing this to a extension method, with this IMutableDependencyResolver
-- that way you can register it against different containers
Then it becomes
Locator.CurrentMutable.UseNLogWithWrappingFullLogger();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,17 @@ | |||
<Project Sdk="MSBuild.Sdk.Extras"> | |||
<PropertyGroup> | |||
<TargetFrameworks>net461;uap10.0.16299;MonoAndroid80;Xamarin.iOS10;netstandard2.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only other suggestion I have is adding the platforms from the main Splat project. Not sure if its strictly necessary though since in theory I think .netstandard2.0/uap/net461 will be likely the only ones needed.
Awesome thanks @dpvreony |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Brings Splat.NLog into the main Splat repository
What is the current behavior? (You can also link to an open issue here)
Was being maintained externally
What is the new behavior (if this is a feature change)?
What might this PR break?
The build until the nuget package permissions are transferred. (not changed the build script)
Please check if the PR fulfills these requirements
Other information: