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

Let IHostBuilder AddNLog work correctly with ${configsetting} #484

Merged
merged 1 commit into from
Sep 28, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Sep 27, 2019

IHostBuilder AddNLog attempt to setup ConfigSettingLayoutRenderer early

Increase the chance that fluent configuration will work.

@snakefoot snakefoot closed this Sep 27, 2019
@snakefoot snakefoot reopened this Sep 27, 2019
@snakefoot snakefoot force-pushed the master branch 2 times, most recently from 7d712e7 to 3444976 Compare September 27, 2019 21:51
@snakefoot snakefoot changed the title IHostBuilder UseNLog attempt to setup ConfigSettingLayoutRenderer early IHostBuilder AddNLog attempt to setup ConfigSettingLayoutRenderer early Sep 27, 2019
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

Please improve then naming and then it's ready to merge :)

@@ -112,8 +112,11 @@ public static LogFactory ConfigureNLog(this ILoggingBuilder builder, LoggingConf
/// <param name="configFileName">Path to NLog configuration file, e.g. nlog.config. </param>
public static ILoggingBuilder AddNLog(this ILoggingBuilder builder, string configFileName)
{
ConfigureServicesNLog(null, builder.Services, serviceProvider => serviceProvider.GetService<IConfiguration>());
LogManager.LoadConfiguration(configFileName);
ConfigureServicesNLog(null, builder.Services, (s, c) =>
Copy link
Member

Choose a reason for hiding this comment

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

Please use longer names then s and c. This isn't C ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a better way that got rid of short names.

@304NotModified
Copy link
Member

Increase the chance that fluent configuration will work.

Could you elaborate this a bit? Not sure what we are to improve here.

@snakefoot
Copy link
Contributor Author

Could you elaborate this a bit? Not sure what we are to improve here.

My initial version of AddNLog loaded the NLog-config before IHostBuilder.Build() had been called. Thus the registration code for setting up config (etc.) had not been called. Causing all Layout-renderers that depends on ${configsetting} to fail.

By delaying the loading of NLog-config until IHostBuilder.Build() is called will help on this.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 28, 2019
@304NotModified 304NotModified changed the title IHostBuilder AddNLog attempt to setup ConfigSettingLayoutRenderer early Let IHostBuilder AddNLog work correctly with ${configsetting} Sep 28, 2019
@304NotModified 304NotModified merged commit 0cde469 into NLog:dev Sep 28, 2019
@304NotModified
Copy link
Member

clear! Thanks!

@snakefoot snakefoot added ASP.NET Core ASP.NET Core - all versions and removed ASP.NET Core 1 labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core ASP.NET Core - all versions enhancement size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants