Add support for multi-dimensional arrays #2035
Conversation
| @@ -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.
Keen not to introduce a new inconsistent set of assertions into this project.
| 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.
Would expect [[a,b],[c,d],[e,f]] here I think?
There was a problem hiding this comment.
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.
| @@ -214,6 +214,29 @@ bool TryConvertEnumerable(object value, Type type, Destructuring destructuring, | |||
| { | |||
| result = SequenceValue.Empty; | |||
| } | |||
| else if (list is Array a && a.Rank > 1) | |||
There was a problem hiding this comment.
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.
IScalarConversionPolicy has no access to
- _depthLimiter (can be solved)
destructuringboolean argument
There was a problem hiding this comment.
In my opinion, we should not use IScalarConversionPolicy here, because the determining factor whether or not using IScalarConversionPolicy is the type of object inside the array, regardless of the dimensions of the array.
| } | ||
| else | ||
| { | ||
| throw new NotSupportedException("Serilog does not support multi-dimensional arrays with Rank > 3."); |
There was a problem hiding this comment.
Since this is not a usage error but just a limitation, perhaps we should just use
result null;
return false;
here?
There was a problem hiding this comment.
Think this is a good candidate to get into 4.0; shall I make these final tweaks and merge this, @sungam3r?
There was a problem hiding this comment.
Thank you for the work on supporting multi-dimensional arrays. ❤️ I hope this feature can be merged soon.
I believe there’s no reason not to support higher-dimensional arrays. Based on this branch, I have further improved the implementation to allow support for arrays with more dimensions: #2132
cc:
@sungam3r
@nblumhardt
|
Committed via #2312 |
fixes #2019