ValueTuple to sequence value conversion #976
Conversation
src/Serilog/Serilog.csproj
Outdated
| <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" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The netstandard1.x targets should probably be updated to just depend on NETStandard.Library anyway, TBH.
There was a problem hiding this comment.
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....
| @@ -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) | |||
There was a problem hiding this comment.
Attempting to keep the cost of the test down.
| definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now. | ||
| { | ||
| var elements = new List<LogEventPropertyValue>(); | ||
| foreach (var field in type.GetTypeInfo().DeclaredFields) |
There was a problem hiding this comment.
Assumes fields are ordered (I think this is safe).
|
Ready to go, if CI likes me :-) |
src/Serilog/Serilog.csproj
Outdated
| @@ -23,25 +23,44 @@ | |||
| <ItemGroup Condition=" '$(TargetFramework)' == 'net45' "> | |||
| <Reference Include="System" /> | |||
| <Reference Include="Microsoft.CSharp" /> | |||
| <PackageReference Include="System.ValueTuple" Version="4.3.1" /> | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
|
Alrighty! Avoided the package dependency and thus any dependency changes. Ready to go 🚀 |
| } | ||
|
|
||
| [Fact] | ||
| public void EightPlusValueTupleElementsAreIgnored() |
There was a problem hiding this comment.
Nit EightPlusValueTupleElementsAreIgnored -> EightPlusValueTupleElementsAreIgnoredWhenDestructuring
Why the arbitrary 8?
There was a problem hiding this comment.
👍 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.
| [Fact] | ||
| public void AllTupleLengthsUpToSevenAreSupported() | ||
| { | ||
| var tuples = new object[] |
There was a problem hiding this comment.
Is an empty array test worthwhile?
There was a problem hiding this comment.
Should we migrate to InlineData for use cases?
There was a problem hiding this comment.
Tried, but ValueTuple isn't regarded by csc as a compile-time constant, so no joy :-(
This PR for #965 converts tuples like
(1, 2, 3)into arrays like[1, 2, 3]for consumption by sinks.I don't think tuples are important enough, currently, to justify the effort of including another
LogEventPropertyValuetype to represent them. Arrays are a bit easier on the eyes than structures, and we don't lose any information since tuple member names aren't available dynamically.The
@operator is not required to capture a tuple. E.g.:is fine. Specifying
@will trigger deep serialization in the case of tuple members that are complex types.The PR is a little ham-fisted (the change doesn't fit in as an
IScalarConversionPolicy, unfortunately, so special treatment is needed). Only tuples up to length 7 are supported.Since tuples are here to stay, and supported everywhere Serilog runs, I've introduced the System.ValueTuple dependency for the current targets. A .NET 4.7-specific target would avoid it, as will .NET Core 2.0. We can add this fresh round of targets once core 2.0 ships, I think (4.7 is a pain right now as it requires a targeting pack).