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.

Sign up for GitHub

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

Jump to bottom

Add support for custom types in arrays and custom collections #201

Closed
wants to merge 8 commits into from
Closed

Add support for custom types in arrays and custom collections #201

wants to merge 8 commits into from

Conversation

sungam3r
Copy link
Contributor

Also:

  • add new test platforms and unify msbuild conditional properties
  • minor code cleanup
  • PackageIcon instead of PackageIconUrl
  • fix for simplified syntax on netcore3.0
  • fix SinkWithConfigurationBindingArgument test (wrong argument type)

sungam3r
sungam3r commented Nov 19, 2019
View reviewed changes
test/Serilog.Settings.Configuration.Tests/Support/JsonStringConfigSource.cs
@@ -45,7 +45,7 @@ public JsonStringConfigProvider(string json) : base(new JsonConfigurationSource

public override void Load()
{
Load(StringToStream(_json));
Load(StringToStream(_json.Fix()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

netcore3.0 System.Text.Json issue

sungam3r
sungam3r commented Nov 19, 2019
View reviewed changes
test/TestDummies/DummyLoggerConfigurationExtensions.cs
@@ -63,13 +63,20 @@ public static LoggerConfiguration WithDummyThreadId(this LoggerEnrichmentConfigu

public static LoggerConfiguration DummyRollingFile(
this LoggerSinkConfiguration loggerSinkConfiguration,
List<string> objectBinding,
List<Binding> objectBinding,
Copy link
Contributor Author

@sungam3r sungam3r Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@nblumhardt In fact, a test using this method should have failed because it sets json incorrectly:

 var json = @"{
                ""Serilog"": {            
                    ""Using"": [""TestDummies""],
                    ""WriteTo"": [{
                        ""Name"": ""DummyRollingFile"",
                        ""Args"": {""pathFormat"" : ""C:\\"",
                                   ""objectBinding"" : [ { ""foo"" : ""bar"" }, { ""abc"" : ""xyz"" } ] }
                    }]        
                }
            }";

objectBinding is not a array of strings, it is array of objects. I came across this error when adding support for arrays of custom types. objectBinding parameter was initialized incorrectly.

sungam3r
sungam3r commented Nov 19, 2019
View reviewed changes
src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs
@@ -262,6 +237,31 @@ string GetSectionName(IConfigurationSection s)
}
}

internal static IConfigurationArgumentValue GetArgumentValue(IConfigurationSection argumentSection, IReadOnlyCollection<Assembly> configurationAssemblies)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it began to be used in another place

sungam3r
sungam3r commented Nov 19, 2019
View reviewed changes
src/Serilog.Settings.Configuration/Settings/Configuration/ObjectArgumentValue.cs
return result;
}

bool TryCreateContainer(out object result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If false fallbacks to ms config binding implementation.

@sungam3r
Copy link
Contributor Author

sungam3r commented Dec 4, 2019

@nblumhardt Could you please find the time to see this PR?

nblumhardt
nblumhardt reviewed Dec 6, 2019
View reviewed changes
test/Serilog.Settings.Configuration.Tests/Support/Extensions.cs
@@ -8,5 +8,15 @@ public static object LiteralValue(this LogEventPropertyValue @this)
{
return ((ScalarValue)@this).Value;
}

// netcore3.0 error:
// Could not parse the JSON file. System.Text.Json.JsonReaderException : ''' is an invalid start of a property name. Expected a '"'
Copy link
Member

Choose a reason for hiding this comment

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

Where do these come from? (Would be better to fix the source JSON if possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Source json does not need to be changed. There are many tests that test the simplified syntax. The problem is not in the tests, but in the fact that the new serializer, in principle, does not support the simplified syntax.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Perhaps ToValidJson() would be more descriptive here? Since it's exploiting some fairly questionable JSON parsing on the earlier platform, we could get rid of the conditional and just use the same method on all platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking changes?

Copy link
Member

Choose a reason for hiding this comment

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

Not that I'm aware of; the code is in the test project (so the changes are on the .NET Core side)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand. The simplified syntax should continue to work. Tests for it should remain, it makes no sense to rewrite them.

just use the same method on all platforms?

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in #203

@nblumhardt
Copy link
Member

Thanks for the nudge @sungam3r - I'm maxed out currently but will try to get back to this shortly, unless another of the maintainers of this repo gets here first :-)

Changes seem good. Just for context, is there a sink we fail to configure currently because of this?

@sungam3r
Copy link
Contributor Author

sungam3r commented Dec 6, 2019

I needed this in my own project. I wanted to configure destructuring policy (not a sink) accepting a list of types (restrictions).

nblumhardt
nblumhardt reviewed Dec 6, 2019
View reviewed changes
test/Serilog.Settings.Configuration.Tests/Serilog.Settings.Configuration.Tests.csproj
<AssemblyName>Serilog.Settings.Configuration.Tests</AssemblyName>
<AssemblyOriginatorKeyFile>../../assets/Serilog.snk</AssemblyOriginatorKeyFile>
<SignAssembly>true</SignAssembly>
<PublicSign Condition=" '$(OS)' != 'Windows_NT' ">true</PublicSign>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net452'">
<PropertyGroup Condition="'$(TargetFramework)' == 'net451' or '$(TargetFramework)' == 'net452'">
Copy link
Member

Choose a reason for hiding this comment

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

net451 isn't in <TargetFrameworks>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in #203

nblumhardt
nblumhardt reviewed Dec 6, 2019
View reviewed changes
src/Serilog.Settings.Configuration/Serilog.Settings.Configuration.csproj
<Authors>Serilog Contributors</Authors>
<TargetFrameworks>netstandard2.0;net451;net461</TargetFrameworks>
<TargetFrameworks>netstandard2.0;net451;net452;net461</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for the additional target framework here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this target because I saw references to it in the conditions.

nblumhardt and others added 5 commits December 7, 2019 19:05
@nblumhardt @sungam3r
Dev version bump [skip ci]
5b67913
@nblumhardt @sungam3r
Clarify role of Microsoft.Extensions.Configuration.Json; resolves ser…
352cd85 
…ilog#181
@nblumhardt @sungam3r
Wrap a (scrolly) long line [skip ci]
a0c5f45
@sungam3r
Add support for custom types in arrays and custom collections
50fd5a9 
add new test platforms and unify msbuild conditional properties
minor code cleanup
PackageIcon instead of PackageIconUrl
fix for simplified syntax on netcore3.0
fix SinkWithConfigurationBindingArgument test (wrong argument type)
@sungam3r
remove parens
63efc49
@sungam3r sungam3r mentioned this pull request Dec 7, 2019
Merged
@sungam3r
Copy link
Contributor Author

sungam3r commented Dec 7, 2019

I accidentally made a rebase on the wrong branch. So I created a new PR - #203

@sungam3r sungam3r closed this Dec 7, 2019
@sungam3r sungam3r deleted the arrays-of-custom-types branch December 7, 2019 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nblumhardt nblumhardt nblumhardt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sungam3r @nblumhardt @tsimbalar