Fixes #97, #83, #98 #100

MV10 · 2018-04-19T17:56:52Z

Fixes #97, adds support for IConfiguration parameters, IConfigurationSection parameters, config-binding of sections by parameter type, and fixes #83, updates MS DependencyModel and Configuration package versions. Probably also addresses #98 though I haven't tested it with arrays specifically. Update: Confirmed, should fix #98.

README updated
GetSection never returns null; corrected null-ref tests to GetChildren().Any() tests
~~Added > "hint" character to identify nested configuration sections~~
Added ObjectArgumentValue class to perform config binding and populate config section parameters
Added two lines of code just before method invocation to populate an IConfiguration parameter

Updated:

Moved nested configuration functionality into ObjectArgumentValue
Removed ConfigurationSectionArgumentValue
Added unit test for IConfiguration and IConfigurationSection arguments
Added unit test for configuration object binding argument (for example, List<>)
Added unit tests for string and int array arguments

SQL Sink PR depends on this:
serilog-mssql/serilog-sinks-mssqlserver#120

sync fork

… execution

nblumhardt

Great PR, thanks! Looks like we're making some substantial progress. Had some questions. Cheers!

nblumhardt · 2018-04-19T22:03:26Z

README.md

@@ -91,3 +91,41 @@ For example, to set the minimum log level using the _Windows_ command prompt:
 set Serilog:MinimumLevel=Debug
 dotnet run
 ```
+
+### Nested configuration sections


Love the additional docs 👍

Updated to remove nested config "hint" hack -- but I left the blurb there about nested config support, figured it can't hurt to document it.

nblumhardt · 2018-04-19T22:05:10Z

README.md

+
+### Nested configuration sections
+
+Some Serilog packages require a reference to a logger configuration object. These must be "hinted" by appending a `>` character to the end of the argument name. The sample program in this project illustrates this with the following entry configuring the _Serilog.Sinks.Async_ package to wrap the _Serilog.Sinks.File_ package.


This seems like a bit of a UX regression, and it's not immediately clear when this will or won't be a breaking change; can't we work out what kind of argument we're dealing with by reflecting over the parameters? I am afraid I'm missing something here, sorry - struggling to keep up :-)

I'll take another look, but other than the Async wrapper package, frankly I'm not clear on the use cases for nested config. (For example, the reason for the logger nested config in the sample code and tests makes no sense to me.) I don't really like the hack myself, but at the point the code is "deciding" how to store the config section (the parameter value, effectively), we don't yet know what the parameter type is. "How to store" means the various parameter-value classes (string, config, or my new object class). I suspect the config value class can go away and that can be added to the object value class.

Fixed. Very glad you questioned this -- the "hint" hack is gone and the code is simplified! I took another look at what was going on in ConfigurationSectionArgumentValue and it turns out I could get rid of that and move the functionality into ObjectArgumentValue. The trick was deferring the new ConfigurationReader pass to read the nested config until we are actually calling the target method. Now the read is triggered by the Action<> parameter's type rather than hinting and/or "assuming" based on config structure.

This made me notice #103

nblumhardt · 2018-04-19T22:05:35Z

README.md

+
+### IConfigurationSection parameters
+
+Certain Serilog packages may require configuration information that can't be easily represented by discrete values or direct binding-friendly representations. An example might be lists of values to remove from a collection of default values. In this case the method can accept an entire `IConfigurationSection` as a call parameter and this package will recognize that and populate the parameter. In this way, Serilog packages can support arbitrarily complex configuration scenarios.


I think in the long run that one is going to be very useful to sink authors.

nblumhardt · 2018-04-19T22:06:01Z

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

        readonly DependencyContext _dependencyContext;
        readonly IReadOnlyCollection<Assembly> _configurationAssemblies;

-        public ConfigurationReader(IConfigurationSection configuration, DependencyContext dependencyContext)
+        #region Constructors


Nit: the Serilog projects don't use #region at all :-)

Removed. (Personally I like them...)

nblumhardt · 2018-04-19T22:07:35Z

test/Serilog.Settings.Configuration.Tests/ConfigurationSettingsTests.cs

@@ -402,7 +402,7 @@ public void WriteToLoggerWithRestrictedToMinimumLevelIsSupported()
                ""WriteTo"": [{
                    ""Name"": ""Logger"",
                    ""Args"": {
-                        ""configureLogger"" : {
+                        ""configureLogger>"" : {


Just a general note on this file - since arbitrary nested config args can be supported, could we include a test here to demonstrate it? I'm worried that it'd be easy to introduce regressions if we don't have at least a smoke test for that option.

That test already checks nested configs (there are two of them). Not clear what else you're asking for. However, it probably does need a test for IConfigurationSection and maybe another for config binding, such as List<T>... I'm still not clear how some of the test "dummy" sinks actually work though, seems overly complicated. I wish there were comments. But I'll have a look.

Tests added and passing on both frameworks, and the revised nested config implementation also passes.

…mentValue, removed hint hack

MV10 · 2018-04-20T19:22:01Z

@nblumhardt Not sure what happened to our comments about that > hack for marking configuration subsections (I've actually never used the PR-review commenting system before with comments associated with the diffs) ... but in case you don't see those comments now, either:

The hack is gone. Basically I got rid of ConfigurationSectionArgumentValue and rolled that into ObjectArgumentValue. The trick was to start the second pass through ConfigurationReader during method invocation, which lets the class recognize the Action<> argument type, rather than the old way that inferred/assumed based on config structure (or the hint hack).

Added unit tests, too, so that covers everything.

nblumhardt · 2018-04-21T22:22:31Z

src/Serilog.Settings.Configuration/Serilog.Settings.Configuration.csproj

+    <GeneratePackageOnBuild>false</GeneratePackageOnBuild>
+    <Version>3.0.0</Version>
+    <AssemblyVersion>3.0.0</AssemblyVersion>
+    <FileVersion>3.0.0</FileVersion>


I don't think these should be necessary - will have a shot now at getting this to build without them, then merge 👍

…og project setup

nblumhardt · 2018-04-21T22:27:27Z

Awesome PR! Just going to check that the build goes through with my minor CSPROJ tweak, then will merge.

The PR comment "disappearing" trick is a bit of an annoying UI default in GitHub - once the code in question has changed, the comments on it are hidden by default.

Cheers!

MV10 · 2018-04-22T11:18:07Z

@nblumhardt Awesome. Entries like "version" sneaked in there because I was using VS to auto-generate local NuGet packages, which made it easier to test with the more complicated config requirements of the SQL sink without actually adding that sink and new sample code to this project. (The transient dependency issue is a real hassle in the .NET Core world where everything is a package now.)

MV10 and others added 9 commits April 12, 2018 05:11

Merge pull request #1 from serilog/dev

bbf9b45

sync fork

minor formatting cleanup; rollback of direct parm value during method…

7e14841

… execution

changed null-ref checks to Any() tests on GetSection results

90b87ba

confing binding and config section hinting

82375c0

populate value for an IConfiguration parameter on target method

80b2ec9

recognize IConfigurationSection parameter types

1239fda

docs for IConfigurationSection

a81d1c6

renamed const to NestedConfigHintChar

60cf699

fixed unit tests with config subsections

da883fa

This was referenced Apr 19, 2018

Configure various ColumnOptions via AppSettings. serilog-mssql/serilog-sinks-mssqlserver#119

Closed

Fixes #108, #111, #99 serilog-mssql/serilog-sinks-mssqlserver#120

Closed

nblumhardt reviewed Apr 19, 2018

View reviewed changes

MV10 added 2 commits April 20, 2018 07:36

moved ConfigurationSectionArgumentValue functionality into ObjectArgu…

400b1de

…mentValue, removed hint hack

unit test for IConfig, IConfigSection, and config object-binding

f9754e4

MV10 mentioned this pull request Apr 20, 2018

Minor issue relating to ConfigurationSection.Value property #103

Closed

unit tests for string/int array arguments

0b3b661

MV10 changed the title ~~Addresses #97 and #83~~ Addresses #97 and #83 and #98 Apr 20, 2018

MV10 changed the title ~~Addresses #97 and #83 and #98~~ Fixes #97, #83, #98 Apr 20, 2018

nblumhardt reviewed Apr 21, 2018

View reviewed changes

Try removing pinned <Version> properties, etc, to match general Seril…

0116625

…og project setup

nblumhardt merged commit 62acc84 into serilog:dev Apr 21, 2018

MV10 deleted the iconfig_parameter branch April 22, 2018 11:27

MV10 mentioned this pull request May 2, 2018

Extension method specially designed for JSON #53

Closed


		### Nested configuration sections

		Some Serilog packages require a reference to a logger configuration object. These must be "hinted" by appending a `>` character to the end of the argument name. The sample program in this project illustrates this with the following entry configuring the _Serilog.Sinks.Async_ package to wrap the _Serilog.Sinks.File_ package.


		### IConfigurationSection parameters

		Certain Serilog packages may require configuration information that can't be easily represented by discrete values or direct binding-friendly representations. An example might be lists of values to remove from a collection of default values. In this case the method can accept an entire `IConfigurationSection` as a call parameter and this package will recognize that and populate the parameter. In this way, Serilog packages can support arbitrarily complex configuration scenarios.

Fixes #97, #83, #98 #100

Fixes #97, #83, #98 #100

Uh oh!

Conversation

MV10 commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nblumhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MV10 Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MV10 commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nblumhardt commented Apr 21, 2018

Uh oh!

MV10 commented Apr 22, 2018

Uh oh!

Uh oh!

MV10 commented Apr 19, 2018 •

edited

Loading

MV10 Apr 20, 2018 •

edited

Loading