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

Conversation

Projects
None yet
4 participants
@nblumhardt
Copy link
Member

nblumhardt commented May 30, 2017

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 LogEventPropertyValue type 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.:

Log.Information("{T}", (1, 2));

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).

<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.

@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.

@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.

@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....

@@ -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.

@nblumhardt

nblumhardt May 30, 2017

Author Member

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)

This comment has been minimized.

@nblumhardt

nblumhardt May 30, 2017

Author Member

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

nblumhardt added some commits May 31, 2017

@nblumhardt nblumhardt changed the title ValueTuple to sequence value conversion [WIP] ValueTuple to sequence value conversion May 31, 2017

@nblumhardt

This comment has been minimized.

Copy link
Member Author

nblumhardt commented May 31, 2017

Ready to go, if CI likes me :-)

@nblumhardt nblumhardt requested a review from adamchester May 31, 2017

@skomis-mm
Copy link

skomis-mm left a comment

just a question :)

@@ -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.

@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.

@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 👍

@nblumhardt

This comment has been minimized.

Copy link
Member Author

nblumhardt commented Jun 1, 2017

Alrighty! Avoided the package dependency and thus any dependency changes. Ready to go 🚀

}

[Fact]
public void EightPlusValueTupleElementsAreIgnored()

This comment has been minimized.

@merbla

merbla Jun 2, 2017

Contributor

Nit EightPlusValueTupleElementsAreIgnored -> EightPlusValueTupleElementsAreIgnoredWhenDestructuring

Why the arbitrary 8?

This comment has been minimized.

@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.

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

This comment has been minimized.

@merbla

merbla Jun 2, 2017

Contributor

Is an empty array test worthwhile?

This comment has been minimized.

@merbla

merbla Jun 2, 2017

Contributor

Should we migrate to InlineData for use cases?

This comment has been minimized.

@nblumhardt

nblumhardt Jun 3, 2017

Author Member

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

@merbla

merbla approved these changes Jun 2, 2017

Copy link
Contributor

merbla left a comment

A few nits but nothing to stop this moving.

@nblumhardt nblumhardt merged commit 8b68856 into serilog:dev Jun 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nblumhardt nblumhardt referenced this pull request Jun 5, 2017

Merged

2.5.0 Release #980

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.