Added NLog.Extensions.Configuration for ${configsetting} for appsettings.json and others

[snakefoot]

Decided not to call it "appsetting" or "appsettings" as it collides with NLog/NLog#2919

[snakefoot]

@linmasaki Would you mind doing a review? It would be nice to have this layoutrenderer in the official NLog-package.

Made some minor optimizations so the cost of reading many config-settings remains low (not loading appsettings.json-files for every instance).

[snakefoot]

When NLog/NLog#2891 has been merged and the first NLog 4.6 BETA has been released, then the same project will be used for loading NLog LoggingConfiguration from appsettings.json.

[linmasaki]

@linmasaki Would you mind doing a review? It would be nice to have this layoutrenderer in the official NLog-package.

Made some minor optimizations so the cost of reading many config-settings remains low (not loading appsettings.json-files for every instance).

No, I wouldn't mind. I also think it would be nice that official NLog-package have this layout renderer.

I will continue to optimize NLog.Appsettings.Standard package. Thank you for your advice!

[304NotModified]

Decided not to call it "appsetting" or "appsettings" as it collides with NLog/NLog#2919

Another option is to overwrite the AppSettingLayoutRenderer in NLog/NLog#2919, and use it as fallback. (could make the fallback even optional). What do you think? I think it's pity it's not called ${appsettings}

[snakefoot]

@304NotModified I think it's pity it's not called ${appsettings}

And rename is done. Lets hope people knows the difference between ${appsetting} and ${appsettings}

[304NotModified]

Another option is to overwrite the AppSettingLayoutRenderer in NLog/NLog#2919, and use it as fallback. (could make the fallback even optional). What do you think?

;)

[snakefoot]

Since you can have hybrid applications that can use both App.Config and AppSettings.json, then I don't like that option (to only allow one). But again I'm probably not the primary user. But apparently you are not either :)

[304NotModified]

But again I'm probably not the primary user. But apparently you are not either :)

At my work we aren't using ASP.NET Core currently (not mature enough for us), but probably we start with it this year or next year.

I use ${appsetting} and it would be nice if I could use the same in .NET Core.

Since you can have hybrid applications that can use both App.Config and AppSettings.json

Yes, therefor the fallback proposal (or an other option is to configure the source).

Since you can have hybrid applications that can use both App.Config and AppSettings.json,

I think that would be rare. For example, the new app.net core application don't use the app.config. Also it's high unlikely to have an application that is both .NET and .NET Core? Or am I missing something?

[snakefoot]

Not sure I understand how this "fallback" is going to work. Already wary of having dependency on json + xml + ini. Think it would be even worse if it had a dependency on "System.Configuration.ConfigurationManager".

Also it's high unlikely to have an application that is both .NET and .NET Core? Or am I missing something?

Seen several of these applications. Which runs net461 but uses NetStandard2.0 libraries. So main application can read app.config-settings while the libraries reads appsettings-json-settings.

[snakefoot]

Maybe one should consider adding a settings-source-repository in NLog, where one could register all kind of sources (with some kind of priority).

Just like one can have candidate config-file-locations, then one could have candidate setting-source-locations. And NLog-extension-assembly could add itself as candidate.

[304NotModified]

There might be some confusion whether it is "appsetting" or "appsettings". See also NLog/NLog#2919

Was thinking about this. It's also pity I confused appsetting and appsettings ;)

So I was thinking to rename it to "appsettings-json". Any thoughts?

[snakefoot]

So I was thinking to rename it to "appsettings-json". Any thoughts?

It is a rather restrictive-name for what it can actually do. I was thinking that the main-program loaded the IConfigurationRoot and assigned it to DefaultConfigurationRoot of this layoutrenderer. Maybe even add a dependency to this in NLog.Web.AspNetCore that will provide an extension method to do DefaultConfigurationRoot assignment.

[304NotModified]

It is a rather restrictive-name

true,

but unfortunately appsetting(s) will clash with NLog/NLog#1948, where I will add the alias appsettings for appsetting

Also this read any config and not appsettings.json isn't? So new proposal: ${configuration-item} :)

[snakefoot]

So new proposal: ${configuration-item} :)

So my initial suggest ${configsetting} is actually good enough?

[304NotModified]

After some thinking, yes :)

Unfortunately it's a force push, if the rename was a separate commit, you could have removed the commit ;)

[304NotModified]

PS: From the viewpoint of reviewing, force pushes are also a bit unhandy. it's unclear what is already reviewed and github shows sometimes "ghost" updates.

I prefer no force pushes and squash it at the end. You could squash it, I can do it when merging.

[codecov]

Codecov Report

Merging #245 into master will decrease coverage by 1.67%.
The diff coverage is 58.06%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
- Coverage   71.87%   70.19%   -1.68%     
==========================================
  Files           8        9       +1     
  Lines         448      510      +62     
  Branches      112      129      +17     
==========================================
+ Hits          322      358      +36     
- Misses         84       99      +15     
- Partials       42       53      +11

Impacted Files	Coverage Δ
...sions.Configuration/ConfigSettingLayoutRenderer.cs	58.06% <58.06%> (ø)

---

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 b37f5ac...452eae2. Read the comment docs.

[snakefoot]

Well I have an OCD about unnecessary noise. And if I have the option to remove it, then I do it :)

I usually only use multiple commits to signal isolated steps.

[304NotModified]

What's noise in your opinion? Commits as try1, try2 etc?

[snakefoot]

What's noise in your opinion? Commits as try1, try2 etc?

Anything that is not the code being added to the final commit :)

[304NotModified]

yeah, so squash the PR is good enough?

[snakefoot]

yeah, so squash the PR is good enough?

Yes if one remember to do it before the merge. But since I don't control the merge step, then I just squash myself :)

[304NotModified]

OK I will keep an eye on it too. I prefer squash on PR merge anyway. (unless it's a backport)

[snakefoot]

Sometimes I like to squash when just removing noise. Sometimes I like separate commits when isolated steps to reach the goal.

[304NotModified]

added to https://nlog-project.org/config and the wiki

direct links:

• https://nlog-project.org/config/?tab=layout-renderers&search=configsetting
• https://github.com/NLog/NLog/wiki/ConfigSetting-Layout-Renderer