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
@@ -162,6 +162,29 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
}
}

if (TryConvertEnumerable(value, destructuring, valueType, limiter, out var enumerableResult))
return enumerableResult;

if (TryConvertValueTuple(value, destructuring, valueType, limiter, out var tupleResult))
return tupleResult;

if (destructuring == Destructuring.Destructure)
{
var type = value.GetType();
var typeTag = type.Name;
if (typeTag.Length <= 0 || IsCompilerGeneratedType(type))
{
typeTag = null;
}

return new StructureValue(GetProperties(value, limiter), typeTag);
}

return new ScalarValue(value.ToString());
}

bool TryConvertEnumerable(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result)
{
var enumerable = value as IEnumerable;
if (enumerable != null)
{
@@ -179,30 +202,69 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur
var keyProperty = typeInfo.GetDeclaredProperty("Key");
var valueProperty = typeInfo.GetDeclaredProperty("Value");

return new DictionaryValue(enumerable.Cast<object>().Take(_maximumCollectionCount)
result = new DictionaryValue(enumerable
.Cast<object>()
.Take(_maximumCollectionCount)
.Select(kvp => new KeyValuePair<ScalarValue, LogEventPropertyValue>(
(ScalarValue)limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring),
limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring)))
(ScalarValue) limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring),
limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring)))
.Where(kvp => kvp.Key.Value != null));
return true;
}

return new SequenceValue(
enumerable.Cast<object>().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring)));
result = new SequenceValue(enumerable
.Cast<object>()
.Take(_maximumCollectionCount)
.Select(o => limiter.CreatePropertyValue(o, destructuring)));

return true;
}

if (destructuring == Destructuring.Destructure)
result = null;
return false;
}

static bool TryConvertValueTuple(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result)
{
if (!(value is IStructuralEquatable && valueType.IsConstructedGenericType))
{
var type = value.GetType();
var typeTag = type.Name;
if (typeTag.Length <= 0 || IsCompilerGeneratedType(type))
result = null;
return false;
}

var definition = valueType.GetGenericTypeDefinition();

// Ignore the 8+ value case for now.
#if VALUETUPLE
if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) ||
definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) ||
definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) ||
definition == typeof(ValueTuple<,,,,,,>))
#else
var defn = definition.FullName;
if (defn == "System.ValueTuple`1" || defn == "System.ValueTuple`2" ||
defn == "System.ValueTuple`3" || defn == "System.ValueTuple`4" ||
defn == "System.ValueTuple`5" || defn == "System.ValueTuple`6" ||
defn == "System.ValueTuple`7")
#endif
{
var elements = new List<LogEventPropertyValue>();
foreach (var field in valueType.GetTypeInfo().DeclaredFields)
{
typeTag = null;
if (field.IsPublic && !field.IsStatic)
{
var fieldValue = field.GetValue(value);
var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring);
elements.Add(propertyValue);
}
}

return new StructureValue(GetProperties(value, limiter), typeTag);
result = new SequenceValue(elements);
return true;
}

return new ScalarValue(value.ToString());
result = null;
return false;
}

LogEventPropertyValue Stringify(object value)
@@ -44,18 +44,6 @@
<PackageReference Include="System.Threading" Version="4.0.11" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net45' ">
<DefineConstants>$(DefineConstants);REMOTING;HASHTABLE</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net46' ">
<DefineConstants>$(DefineConstants);ASYNCLOCAL</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<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" />
@@ -71,4 +59,16 @@
<PackageReference Include="System.Threading" Version="4.0.11" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net45' ">
<DefineConstants>$(DefineConstants);REMOTING;HASHTABLE</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net46' ">
<DefineConstants>$(DefineConstants);ASYNCLOCAL</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<DefineConstants>$(DefineConstants);ASYNCLOCAL;HASHTABLE</DefineConstants>
</PropertyGroup>

</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,55 @@ 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]);
}

[Fact]
public void AllTupleLengthsUpToSevenAreSupported()
{
var tuples = new object[]

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Is an empty array test worthwhile?

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Should we migrate to InlineData for use cases?

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 3, 2017

Author Member

Tried, but ValueTuple isn't regarded by csc as a compile-time constant, so no joy :-(

{
ValueTuple.Create(1),
(1, 2),
(1, 2, 3),
(1, 2, 3, 4),
(1, 2, 3, 4, 5),
(1, 2, 3, 4, 5, 6),
(1, 2, 3, 4, 5, 6, 7)
};

foreach (var t in tuples)
Assert.IsType<SequenceValue>(_converter.CreatePropertyValue(t));
}

[Fact]
public void EightPlusValueTupleElementsAreIgnored()

This comment has been minimized.

Copy link
@merbla

merbla Jun 2, 2017

Contributor

Nit EightPlusValueTupleElementsAreIgnored -> EightPlusValueTupleElementsAreIgnoredWhenDestructuring

Why the arbitrary 8?

This comment has been minimized.

Copy link
@nblumhardt

nblumhardt Jun 3, 2017

Author Member

👍 probably should be WhenCapturing, but good point.

The limitation is in how ValueTuple works, up to seven elements can be represented directly in one tuple, but to go 8+ they need to be nested. No major issue with implementing it down the road.

{
var scalar = _converter.CreatePropertyValue((1, 2, 3, 4, 5, 6, 7, 8));
Assert.IsType<ScalarValue>(scalar);
}

[Fact]
public void DestructuringIsTransitivelyApplied()
{
var tuple = _converter.CreatePropertyValue(ValueTuple.Create(new {A = 1}), true);
var sequence = Assert.IsType<SequenceValue>(tuple);
Assert.IsType<StructureValue>(sequence.Elements[0]);
}
}
}

@@ -18,6 +18,7 @@
<PackageReference Include="Newtonsoft.Json" Version="9.0.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.2.0" />
<PackageReference Include="xunit" Version="2.2.0" />
<PackageReference Include="System.ValueTuple" Version="4.3.1" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">
</ItemGroup>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.