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

ValueTuple to sequence value conversion #976

Merged
merged 6 commits into from Jun 3, 2017
Merged
Diff settings

Always

Just for now

@@ -74,7 +74,7 @@ partial class PropertyValueConverter : ILogEventPropertyFactory, ILogEventProper
new SimpleScalarConversionPolicy(BuiltInScalarTypes.Concat(additionalScalarTypes)),
new NullableScalarConversionPolicy(),
new EnumScalarConversionPolicy(),
new ByteArrayScalarConversionPolicy(),
new ByteArrayScalarConversionPolicy()
};

_destructuringPolicies = additionalDestructuringPolicies
@@ -190,6 +190,33 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
enumerable.Cast<object>().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring)));
}

if (value is IStructuralEquatable)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt May 30, 2017

Author Member

Attempting to keep the cost of the test down.

{
var type = value.GetType();
if (type.IsConstructedGenericType)
{
var definition = type.GetGenericTypeDefinition();
if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) ||
definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) ||
definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) ||
definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now.
{
var elements = new List<LogEventPropertyValue>();
foreach (var field in type.GetTypeInfo().DeclaredFields)

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt May 30, 2017

Author Member

Assumes fields are ordered (I think this is safe).

{
if (field.IsPublic && !field.IsStatic)
{
var fieldValue = field.GetValue(value);
var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring);
elements.Add(propertyValue);
}
}

return new SequenceValue(elements);
}
}
}

if (destructuring == Destructuring.Destructure)
{
var type = value.GetType();
Copy path View file
@@ -23,25 +23,44 @@
<ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />

This comment has been minimized.

Copy link
@skomis-mm

skomis-mm May 31, 2017

It's a pity. Why you didn't considered the fourth option (along with the first)?

4.Support it everywhere, using reflection only (no direct dependency on ValueTuple itself)

Is that because of some performance concerns? I believe you could check type.Name to constants (ValueTuple'1, ValueTuple'2 etc) like you did with definition == comparisons

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 1, 2017

Author Member

Thanks for the suggestion. I initially discounted this because it seemed a bit awkward, given tuples are a feature of C# that's here to stay. On second thought, it's not much worse to do the type name check than to do what we have here, so I'll switch it back over and #if VALUETUPLE conditionally-include the Type-based version 👍

</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net46' ">
<Reference Include="System" />
<Reference Include="Microsoft.CSharp" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.0' ">
<PackageReference Include="Microsoft.CSharp" Version="4.0.1" />
<PackageReference Include="System.Collections" Version="4.0.11" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.0.11" />
<PackageReference Include="System.Globalization" Version="4.0.11" />
<PackageReference Include="System.Linq" Version="4.1.0" />
<PackageReference Include="System.Reflection" Version="4.1.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.0.1" />
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.1.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.1.0" />
<PackageReference Include="System.Threading" Version="4.0.11" />
<PackageReference Include="Microsoft.CSharp" Version="4.3.0" />

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt May 30, 2017

Author Member

These dependencies are updated; System.ValueTuple depends on s.Collections under .NET Standard, and requires at least 4.3.0, so we might as well be consistent.

This comment has been minimized.

Copy link
@khellang

khellang May 31, 2017

Member

The netstandard1.x targets should probably be updated to just depend on NETStandard.Library anyway, TBH.

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 1, 2017

Author Member

Thanks @khellang - you're probably right here. I guess the dust has settled enough for us to embrace the new order of things :-). Perhaps we make the change when .NET Core 2.0 comes along? I am tempted to leave it alone in this PR, just to avoid any more chasing of corner cases....

<PackageReference Include="System.Collections" Version="4.3.0" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />
<PackageReference Include="System.Globalization" Version="4.3.0" />
<PackageReference Include="System.Linq" Version="4.3.0" />
<PackageReference Include="System.Reflection" Version="4.3.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.3.0" />
<PackageReference Include="System.Runtime" Version="4.3.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.3.0" />
<PackageReference Include="System.Threading" Version="4.3.0" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<PackageReference Include="System.Collections.NonGeneric" Version="4.3.0" />
<PackageReference Include="Microsoft.CSharp" Version="4.3.0" />
<PackageReference Include="System.Collections" Version="4.3.0" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />
<PackageReference Include="System.Globalization" Version="4.3.0" />
<PackageReference Include="System.Linq" Version="4.3.0" />
<PackageReference Include="System.Reflection" Version="4.3.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.3.0" />
<PackageReference Include="System.Runtime" Version="4.3.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.3.0" />
<PackageReference Include="System.Threading" Version="4.3.0" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net45' ">
@@ -56,19 +75,4 @@
<DefineConstants>$(DefineConstants);ASYNCLOCAL;HASHTABLE</DefineConstants>
</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<PackageReference Include="System.Collections.NonGeneric" Version="4.0.1" />
<PackageReference Include="Microsoft.CSharp" Version="4.0.1" />
<PackageReference Include="System.Collections" Version="4.0.11" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.0.11" />
<PackageReference Include="System.Globalization" Version="4.0.11" />
<PackageReference Include="System.Linq" Version="4.1.0" />
<PackageReference Include="System.Reflection" Version="4.1.0" />
<PackageReference Include="System.Reflection.Extensions" Version="4.0.1" />
<PackageReference Include="System.Runtime" Version="4.1.0" />
<PackageReference Include="System.Runtime.Extensions" Version="4.1.0" />
<PackageReference Include="System.Text.RegularExpressions" Version="4.1.0" />
<PackageReference Include="System.Threading" Version="4.0.11" />
</ItemGroup>

</Project>
@@ -8,6 +8,8 @@
using Serilog.Parsing;
using Serilog.Tests.Support;

// ReSharper disable UnusedAutoPropertyAccessor.Global, UnusedParameter.Local

namespace Serilog.Tests.Parameters
{
public class PropertyValueConverterTests
@@ -188,7 +190,7 @@ public class BaseWithProps

public class DerivedWithOverrides : BaseWithProps
{
new public string PropA { get; set; }
public new string PropA { get; set; }
public override string PropB { get; set; }
public string PropD { get; set; }
}
@@ -216,7 +218,7 @@ public void NewAndInheritedPropertiesAppearOnlyOnce()

class HasIndexer
{
public string this[int index] { get { return "Indexer"; } }
public string this[int index] => "Indexer";
}

[Fact]
@@ -231,7 +233,7 @@ public void IndexerPropertiesAreIgnoredWhenDestructuring()
// (reducing garbage).
class HasItem
{
public string Item { get { return "Item"; } }
public string Item => "Item";
}

[Fact]
@@ -253,6 +255,22 @@ public void CSharpAnonymousTypesAreRecognizedWhenDestructuring()
var structuredValue = (StructureValue)result;
Assert.Equal(null, structuredValue.TypeTag);
}

[Fact]
public void ValueTuplesAreRecognizedWhenDestructuring()
{
var o = (1, "A", new[] { "B" });
var result = _converter.CreatePropertyValue(o);

var sequenceValue = Assert.IsType<SequenceValue>(result);

Assert.Equal(3, sequenceValue.Elements.Count);
Assert.Equal(new ScalarValue(1), sequenceValue.Elements[0]);
Assert.Equal(new ScalarValue("A"), sequenceValue.Elements[1]);
var nested = Assert.IsType<SequenceValue>(sequenceValue.Elements[2]);
Assert.Equal(1, nested.Elements.Count);
Assert.Equal(new ScalarValue("B"), nested.Elements[0]);
}
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.