Codecov Report

Merging #2624 into master will decrease coverage by <1%.
The diff coverage is 76%.

@@           Coverage Diff           @@
##           master   #2624    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         326     327     +1     
  Lines       24034   24294   +260     
  Branches     3037    3088    +51     
=======================================
+ Hits        19443   19561   +118     
- Misses       3751    3889   +138     
- Partials      840     844     +4

Could you please tell me what's new code, (no moves), makes it easier to review, thanks!

Well before you start the review, then you should read the initial comment. Which I will repeat again. Is the Json-config shown in #1588 (comment) acceptable? (Along with all the restrictions)

Well before you start the review, then you should read the initial comment. Which I will repeat again. Is the Json-config shown in #1588 (comment) acceptable? (Along with all the restrictions)

yes, I saw it. Have to think about the restrictions. .e.g if a variable is using another variable, that could be an issue (now it's dependent of the order)

Anyway, splitting the config reader from the XML config reader it a good step anyway.

.e.g if a variable is using another variable, that could be an issue (now it's dependent of the order)

Well the variables in the variables-section are parsed in their correct order. It is only the individual sections that are parsed in alphabetic order. (Ex variables-section are parsed last because it starts with "v", so like extensions-section it has to be pre-loaded).

I think the targets config in the proposed JSON looks awesome! Would it be possible to change the "internalLog" and "extensions" section as in the opening post? And how should we configure rules without names?

how should we configure rules without names?

The same way you configure targets without names. You don't :)

Well the variables in the variables-section are parsed in their correct order.

So this goes well?

 "variables": {
            "rootDir": "c:/somewhere"
            "basedirArchives": "${rootDir}/archives",
            "basedirFiles": "${rootDir}/files",
        },

I a bit confused why that one isn't parsed alphabeticly. It it because "extentions" etc is the the intermediate child of the root? IMO Json are just properties and arrays.

The same way you configure targets without names. You don't :)

That will be hard for users to come up with unique names while they don't use it.

Would it be possible to change the "internalLog" section as in the opening post?

I guess it is possible to fiddle with the LoggingConfigurationReader, so can support reading internalLog-parameters from a section different from the nlog-section.

Would it be possible to change the "extensions" section as in the opening post?

Nope. This Json is ignored by the microsoft json parser:

"NLog.Extended": {},

That will be hard for users to come up with unique names while they don't use it.

Same can be said for wrapped targets :). Also the names doesn't have to be unique. Just like the target-names (But maybe the Json-protocol will limit you).

Same can be said for wrapped targets :)

true, but wrapped targets are less common/basic and now we have already unnamed rules.

There is also no support for autoReload or including external json-files.

There is reloadOnChange (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?tabs=basicconfiguration)

There is reloadOnChange

Sounds like you are ready to take over the PR, and fiddle with how to hook into the reloadOnChange-event and perform a controlled reload if something happened to the NLog-section.

true, but wrapped targets are less common/basic and now we have already unnamed rules.

I guess the logging-rules could be implemented like the extensions-section-array:

        "rules": [
            {
                "logger": "*",
                "minLevel": "Trace",
                "writeTo": "SplitGroup"
            },
            {
                "logger": "*",
                "minLevel": "Trace",
                "writeTo": "postFilteringWrapper"
            },
        ],

I just liked that the rules and targets where using the same syntax.

Sounds like you are ready to take over the PR, and fiddle with how to hook into the reloadOnChange-event and perform a controlled reload if something happened to the NLog-section.

I will try, but I don't expect if have time for that before the weekend.

I guess the logging-rules could be implemented like the extensions-section-array:

👍 nice!

looks like I have an action to do here :)

I think this could be merged for 4.6, independent of the discussion of JSON config files, do you agree?

There is a pure refactoring, isn't ? I like to accept it.

There is a pure refactoring, isn't ? I like to accept it.

No it is a bad attempt to reuse the XML-logic for reading JSON-files.

The reordering logic (because JSON-keys are ordered) need happen in the JSON-parser (and not in the reader).

The variable _xmlConfigMode need to be removed.

No it is a bad attempt to reuse the XML-logic for reading JSON-files.

OK, I think I close it then (for now)