-
Notifications
You must be signed in to change notification settings - Fork 797
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
Add support for multi-dimensional arrays #2035
base: dev
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,6 +214,29 @@ bool TryConvertEnumerable(object value, Type type, Destructuring destructuring, | |
{ | ||
result = SequenceValue.Empty; | ||
} | ||
else if (list is Array a && a.Rank > 1) | ||
{ | ||
int index = 0; | ||
var array = new LogEventPropertyValue[list.Count]; | ||
if (a.Rank == 2) | ||
{ | ||
for (int i = 0; i < a.GetLength(0); ++i) | ||
sungam3r marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (int j = 0; j < a.GetLength(1); ++j) | ||
array[index++] = _depthLimiter.CreatePropertyValue(a.GetValue(i, j), destructuring); | ||
} | ||
else if (a.Rank == 3) | ||
{ | ||
for (int i = 0; i < a.GetLength(0); ++i) | ||
for (int j = 0; j < a.GetLength(1); ++j) | ||
for (int k = 0; k < a.GetLength(2); ++k) | ||
array[index++] = _depthLimiter.CreatePropertyValue(a.GetValue(i, j, k), destructuring); | ||
} | ||
else | ||
{ | ||
throw new NotSupportedException("Serilog does not support multi-dimensional arrays with Rank > 3."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is not a usage error but just a limitation, perhaps we should just use
here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think this is a good candidate to get into 4.0; shall I make these final tweaks and merge this, @sungam3r? |
||
} | ||
result = new SequenceValue(array); | ||
} | ||
else | ||
{ | ||
var array = new LogEventPropertyValue[list.Count]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using Shouldly; | ||
using System.Diagnostics; | ||
|
||
#pragma warning disable Serilog004 // Constant MessageTemplate verifier | ||
|
@@ -317,6 +318,54 @@ public void NullMessageTemplateParametersDoNotBreakBinding() | |
// ReSharper restore StructuredMessageTemplateProblem | ||
} | ||
|
||
// https://github.com/serilog/serilog/issues/2019 | ||
[Fact] | ||
public void Two_Dimensional_Array_Should_Be_Logger_As_Sequence() | ||
{ | ||
var evt = DelegatingSink.GetLogEvent(l => | ||
{ | ||
var a = new object[3, 2] { { "a", "b" }, { "c", "d" }, { "e", "f" } }; | ||
l.Error("{@Value}", a); | ||
}); | ||
|
||
evt.Properties.Count.ShouldBe(1); | ||
var arr = evt.Properties["Value"].ShouldBeOfType<SequenceValue>(); | ||
arr.Elements.Count.ShouldBe(6); | ||
arr.LiteralValue().ShouldBe("[a,b,c,d,e,f]"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did it intentionally since any array is just a sequence in terms of serilog regardless dimensions. Nevertheless I can change implementation to emit sequence of sequences if you like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
|
||
// https://github.com/serilog/serilog/issues/2019 | ||
[Fact] | ||
public void Three_Dimensional_Array_Should_Be_Logger_As_Sequence() | ||
{ | ||
var evt = DelegatingSink.GetLogEvent(l => | ||
{ | ||
var a = new object[3, 2, 1] { { { "a" }, { "b" } }, { { "c" }, { "d" } }, { { "e" }, { "f" } } }; | ||
l.Error("{@Value}", a); | ||
}); | ||
|
||
evt.Properties.Count.ShouldBe(1); | ||
var arr = evt.Properties["Value"].ShouldBeOfType<SequenceValue>(); | ||
arr.Elements.Count.ShouldBe(6); | ||
arr.LiteralValue().ShouldBe("[a,b,c,d,e,f]"); | ||
} | ||
|
||
|
||
// https://github.com/serilog/serilog/issues/2019 | ||
[Fact] | ||
public void Four_Dimensional_Array_Not_Supported() | ||
{ | ||
var evt = DelegatingSink.GetLogEvent(l => | ||
{ | ||
var a = new object[4, 3, 2, 1]; | ||
l.Error("{@Value}", a); | ||
}); | ||
|
||
evt.Properties.Count.ShouldBe(1); | ||
var val = evt.Properties["Value"].ShouldBeOfType<ScalarValue>(); | ||
val.LiteralValue().ShouldBe("Capturing the property value threw an exception: NotSupportedException"); | ||
} | ||
|
||
#if FEATURE_ASYNCDISPOSABLE | ||
|
||
[Fact] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<!-- Tests target .NET Framework 4.6.2 plus the latest RTM of .NET Framework --> | ||
<TargetFrameworks Condition=" '$(OS)' == 'Windows_NT'">net462;net48</TargetFrameworks> | ||
|
@@ -21,6 +21,7 @@ | |
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.3" PrivateAssets="all" /> | ||
<PackageReference Include="xunit" Version="2.4.2" /> | ||
<PackageReference Include="System.ValueTuple" Version="4.5.0" /> | ||
<PackageReference Include="Shouldly" Version="4.1.0" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keen not to introduce a new inconsistent set of assertions into this project. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
</ItemGroup> | ||
|
||
<PropertyGroup Condition=" '$(TargetFramework)' == 'net462' "> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be plugged in as an
IScalarConversionPolicy
instead? Since those are checked before this code is reached, we could avoid the extra condition here. We do this already to specialize conversion of byte arrays.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IScalarConversionPolicy
has no access todestructuring
boolean argumentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, we should not use
IScalarConversionPolicy
here, because the determining factor whether or not usingIScalarConversionPolicy
is the type of object inside the array, regardless of the dimensions of the array.