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

Improve AddNLog with LoggingConfiguration to handle custom LogFactory #407

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Mar 16, 2020

Improves handling of #406

@snakefoot snakefoot added this to the 1.6.2 milestone Mar 16, 2020
@codecov-io
Copy link

Codecov Report

Merging #407 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   82.07%   82.08%   +0.01%     
==========================================
  Files          12       12              
  Lines        1177     1178       +1     
  Branches      193      194       +1     
==========================================
+ Hits          966      967       +1     
  Misses        141      141              
  Partials       70       70
Impacted Files Coverage Δ
...tensions.Logging/Extensions/ConfigureExtensions.cs 56.36% <100%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0466a9c...eb8a90f. Read the comment docs.

@snakefoot
Copy link
Contributor Author

Wow that was quick :)

@304NotModified
Copy link
Member

@snakefoot should we also change

public static LoggingConfiguration ConfigureNLog(this ILoggerFactory loggerFactory, LoggingConfiguration config)
{
LogManager.AddHiddenAssembly(typeof(NLogLoggerProvider).GetTypeInfo().Assembly);
LogManager.Configuration = config;
return config;
}

?

@304NotModified
Copy link
Member

Wow that was quick :)

quick build times :)

@snakefoot
Copy link
Contributor Author

Since ConfigureNLog are obsolete methods, then I see no need to change their behavior.

@304NotModified
Copy link
Member

OK was also doubting about that. Unfortunately obsolete methods stay a bit long on highly used projects ;)

@snakefoot
Copy link
Contributor Author

Well nothing like a good carrot to lure them over the non-obsolete methods.

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

Successfully merging this pull request may close these issues.

2 IServiceProvider instances with 2 log configs causes 1 instance to use wrong config.
3 participants